EOA Production Readiness #14
Conversation
WalkthroughThe entire previous EOA executor store implementation was replaced by a new modular Redis-backed store with atomic transaction support and refined concurrency control. The EOA worker was updated to use this atomic store, with transaction lifecycle states renamed and flows refactored for explicit credential handling, improved locking, and error classification. The execution router and queue manager were adapted to use Redis connection managers and namespaces instead of a shared store instance. New webhook event structures and queuing functions were added to support notification workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutionRouter
participant EoaExecutorWorker
participant AtomicEoaExecutorStore
participant Chain
participant Redis
Client->>ExecutionRouter: execute_eoa(request)
ExecutionRouter->>EoaExecutorWorker: Submit EOA job (with noop signing credential)
EoaExecutorWorker->>AtomicEoaExecutorStore: acquire_eoa_lock_aggressively(lease_token)
AtomicEoaExecutorStore->>Redis: WATCH keys and validate lock ownership
EoaExecutorWorker->>AtomicEoaExecutorStore: get_submitted_transactions_count()
alt Work remains
EoaExecutorWorker->>Chain: send transaction (signed with explicit credential)
EoaExecutorWorker->>AtomicEoaExecutorStore: atomic_move_borrowed_to_submitted()
EoaExecutorWorker->>AtomicEoaExecutorStore: add_gas_bump_attempt() / update_health_data()
else No work
EoaExecutorWorker->>AtomicEoaExecutorStore: release_eoa_lock()
end
EoaExecutorWorker->>AtomicEoaExecutorStore: clean_submitted_transactions()
AtomicEoaExecutorStore->>Redis: MULTI/EXEC atomic cleanup pipeline
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)error: failed to get Caused by: Caused by: Caused by: Caused by: 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
executors/src/eoa/worker.rs (2)
759-786: Consider extracting balance update logic into a separate method.While the optimization to reduce RPC calls is good, the nested conditionals and TODO comment indicate this needs refactoring. Consider extracting this into a dedicated method for clarity:
- // TODO: refactor this, very ugly - if health.balance <= health.balance_threshold { - if now - health.balance_fetched_at > HEALTH_CHECK_INTERVAL { - let balance = chain - .provider() - .get_balance(scoped.eoa()) - .await - .map_err(|e| { - let engine_error = e.to_engine_error(chain); - EoaExecutorWorkerError::RpcError { - message: format!("Failed to get balance: {}", engine_error), - inner_error: engine_error, - } - })?; - - health.balance = balance; - health.balance_fetched_at = now; - scoped.update_health_data(&health).await?; - } - - if health.balance <= health.balance_threshold { - tracing::warn!( - "EOA has insufficient balance (<= {} wei), skipping send flow", - health.balance_threshold - ); - return Ok(0); - } + // Update balance if needed and check if sufficient + if !self.ensure_sufficient_balance(&mut health, scoped, chain, now).await? { + tracing::warn!( + "EOA has insufficient balance (<= {} wei), skipping send flow", + health.balance_threshold + ); + return Ok(0); }Then implement
ensure_sufficient_balanceas a separate method that handles the balance fetching logic cleanly.
526-527: Performance opportunity: Batch Redis operations as noted in TODO.The TODO correctly identifies that sequential Redis operations for state transitions could be batched for better performance. This would reduce round trips to Redis and improve throughput.
Would you like me to help implement batched state transitions for
borrowed -> submittedandborrowed -> recycledoperations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
executors/src/eoa/store.rs(9 hunks)executors/src/eoa/worker.rs(27 hunks)server/src/execution_router/mod.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/worker.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
🧬 Code Graph Analysis (1)
executors/src/eoa/worker.rs (3)
executors/src/eoa/store.rs (4)
nonce(454-459)eoa(2349-2351)from(1988-1992)from(1996-2001)twmq/src/multilane.rs (1)
count(283-347)core/src/error.rs (5)
from(251-268)from(272-276)from(452-456)from(460-464)from(468-472)
🔇 Additional comments (11)
server/src/execution_router/mod.rs (1)
432-432: Clarify the purpose ofnoop_signing_credentialfield name.The field name
noop_signing_credentialis confusing. If this credential is used for actual signing operations in the worker, the "noop" prefix is misleading. Consider renaming it tosigning_credentialor documenting why it's called "noop".executors/src/eoa/worker.rs (10)
14-19: LGTM! Import organization looks good.The addition of
chain::{Chain, ChainService}imports improves code organization by consolidating related imports together.
47-51: Good refactor: Direct credential passing improves security.Replacing
worker_idwithnoop_signing_credentialis a solid improvement that allows direct credential handling for no-op transactions, eliminating the need for credential lookup and improving security.
86-96: Excellent error classification improvement.Separating
TransactionSendErrorfrom genericRpcErrorprovides better granularity for handling transaction broadcasting failures specifically, which improves debugging and error recovery strategies.
246-278: Excellent terminology improvements for transaction states.The renaming from
Pending→SubmittedandFailed→Replacedprovides much clearer semantics:
- "Submitted" accurately reflects transactions that have been broadcast to the network
- "Replaced" is more precise than "Failed" for transactions superseded by others with the same nonce
This improves code readability and reduces confusion about transaction lifecycle states.
338-393: Smart improvement: Using lease_token for lock management.Switching from
worker_idin job data tolease_tokenfrom the job context is a more robust approach. The lease token is managed by the job queue system and ensures proper lock ownership verification, which aligns well with the WATCH-based coordination mentioned in the learnings.
442-456: Good addition: Tracking submitted transactions for complete lifecycle.Including
submitted_countin the work remaining check ensures the worker continues processing until all transactions reach a terminal state (confirmed or replaced). This prevents premature worker termination when transactions are still awaiting confirmation.
1180-1188: Good optimization: Scaled delays prevent nonce conflicts.The change from a fixed delay to
50 * imilliseconds creates a sensible spacing between transaction broadcasts, reducing the likelihood of nonce conflicts while maintaining reasonable throughput.
1343-1377: Clean refactor: Explicit credential passing and simplified no-op transactions.The changes improve the no-op transaction flow by:
- Accepting credentials explicitly rather than relying on implicit data
- Simplifying transaction construction with sensible defaults
- Using the new
TransactionSendErrorfor better error classificationThe implementation is more straightforward and maintainable.
1853-1892: Good separation of concerns in transaction signing.Decoupling the signing process by explicitly passing
fromaddress andcredentialimproves flexibility and makes the signing flow more testable. This allows signing transactions without requiring the full transaction data structure.
1204-1210: Ensure pending transaction handling aligns with the provider’s return typeThe call in
executors/src/eoa/worker.rs(around lines 1204–1210) now does:Ok(pending) => { // Transaction sent successfully match scoped.move_borrowed_to_submitted( prepared.nonce, &pending.tx_hash().to_string(), &prepared.transaction_id,• Please confirm that
chain.provider().send_tx_envelope(...)indeed returns a type exposingtx_hash()(i.e. a pending‐transaction struct) and not, for example, a receipt or unit.
• If the Alloy provider’s API has changed, adjust this extraction or mapping accordingly.
executors/src/eoa/store.rs
Outdated
| for transaction_id in &oldest_transactions { | ||
| let tx_data_key = self.transaction_data_key_name(transaction_id); | ||
| let _: () = conn.del(&tx_data_key).await?; | ||
| } | ||
|
|
||
| // Remove from tracking sets | ||
| for transaction_id in &oldest_transactions { | ||
| let _: () = conn.zrem(&completed_eoa_key, transaction_id).await?; | ||
| let _: () = conn.zrem(&completed_global_key, transaction_id).await?; | ||
| } |
There was a problem hiding this comment.
Batch Redis operations in pruning methods for better performance.
The pruning methods execute individual del and zrem operations in loops, which is inefficient. For better performance, use Redis pipelines to batch these operations.
Apply this pattern to both pruning methods:
- // Remove transaction data and tracking
- for transaction_id in &oldest_transactions {
- let tx_data_key = self.transaction_data_key_name(transaction_id);
- let _: () = conn.del(&tx_data_key).await?;
- }
-
- // Remove from tracking sets
- for transaction_id in &oldest_transactions {
- let _: () = conn.zrem(&completed_eoa_key, transaction_id).await?;
- let _: () = conn.zrem(&completed_global_key, transaction_id).await?;
- }
+ // Batch remove transaction data and tracking
+ let mut pipeline = twmq::redis::pipe();
+ for transaction_id in &oldest_transactions {
+ let tx_data_key = self.transaction_data_key_name(transaction_id);
+ pipeline.del(&tx_data_key);
+ pipeline.zrem(&completed_eoa_key, transaction_id);
+ pipeline.zrem(&completed_global_key, transaction_id);
+ }
+ let _: Vec<twmq::redis::Value> = pipeline.query_async(&mut conn).await?;Also applies to: 1894-1902
🤖 Prompt for AI Agents
In executors/src/eoa/store.rs around lines 1852 to 1861 and also 1894 to 1902,
the code performs individual Redis `del` and `zrem` operations inside loops,
which is inefficient. Refactor these loops to use Redis pipelines by batching
all `del` commands together and all `zrem` commands together in a single
pipeline execution. This will reduce the number of round trips to Redis and
improve performance.
executors/src/eoa/store.rs
Outdated
| }; | ||
|
|
||
| // Get all EOA-specific completed transaction keys | ||
| let keys: Vec<String> = conn.keys(&pattern).await?; |
There was a problem hiding this comment.
Replace KEYS command with SCAN to avoid blocking Redis.
Using keys() can block Redis in production environments with many keys. Use SCAN for non-blocking iteration.
Replace the KEYS usage with SCAN:
- // Get all EOA-specific completed transaction keys
- let keys: Vec<String> = conn.keys(&pattern).await?;
+ // Get all EOA-specific completed transaction keys using SCAN
+ let mut keys = Vec::new();
+ let mut cursor = 0u64;
+ loop {
+ let (new_cursor, batch): (u64, Vec<String>) = twmq::redis::cmd("SCAN")
+ .arg(cursor)
+ .arg("MATCH")
+ .arg(&pattern)
+ .arg("COUNT")
+ .arg(100)
+ .query_async(&mut conn)
+ .await?;
+ keys.extend(batch);
+ cursor = new_cursor;
+ if cursor == 0 {
+ break;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let keys: Vec<String> = conn.keys(&pattern).await?; | |
| // Get all EOA-specific completed transaction keys using SCAN | |
| let mut keys = Vec::new(); | |
| let mut cursor = 0u64; | |
| loop { | |
| let (new_cursor, batch): (u64, Vec<String>) = twmq::redis::cmd("SCAN") | |
| .arg(cursor) | |
| .arg("MATCH") | |
| .arg(&pattern) | |
| .arg("COUNT") | |
| .arg(100) | |
| .query_async(&mut conn) | |
| .await?; | |
| keys.extend(batch); | |
| cursor = new_cursor; | |
| if cursor == 0 { | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
In executors/src/eoa/store.rs at line 1924, replace the blocking Redis KEYS
command with the non-blocking SCAN command. Modify the code to asynchronously
iterate over keys matching the pattern using SCAN, collecting them into a
Vec<String> without blocking Redis, ensuring efficient and safe key retrieval in
production environments.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
executors/src/eoa/store/submitted.rs (1)
188-190: Address the TODO for queuing success jobs.The cleanup operation moves transactions to success state but doesn't queue the success jobs. This could lead to missing webhook notifications or other post-success processing.
Would you like me to implement the success job queuing logic or create an issue to track this task?
executors/src/eoa/store/atomic.rs (1)
259-259: Fix trailing comma in documentation example.Remove the trailing comma after
namespaceparameter.-/// let base_store = EoaExecutorStore::new(redis, namespace, ); +/// let base_store = EoaExecutorStore::new(redis, namespace);executors/src/eoa/store/mod.rs (1)
67-68: Add TODO comment for attempt_number field.The
attempt_numberfield is initialized to 0 and updated later when reading all attempts. Consider adding a TODO comment to make this clearer.pub details: Signed<TypedTransaction>, pub sent_at: u64, // Unix timestamp in milliseconds + // TODO: This is set to 0 initially and updated when reading all attempts pub attempt_number: u32,executors/src/eoa/worker.rs (4)
427-444: Consider using AtomicEoaExecutorStore's release method.The current implementation manually deletes the lock key, but
AtomicEoaExecutorStoreprovides arelease_eoa_lock()method that handles this with proper error handling. However, this would require passing the store instance, which might not be available in the hook context.
492-492: Valid TODO: Batch Redis operations for better performance.The sequential Redis operations could be slow with many borrowed transactions. Consider using Redis pipelines to batch the state transitions.
Would you like me to implement the batched version or create an issue to track this optimization?
694-721: Refactor nested balance checking logic.The balance update logic works but the nested conditions make it hard to follow. Consider extracting to a helper method.
async fn should_update_balance(&self, health: &EoaHealth, now: u64) -> bool { health.balance <= health.balance_threshold && now - health.balance_fetched_at > HEALTH_CHECK_INTERVAL } // Then in send_flow: if self.should_update_balance(&health, now).await { let balance = chain.provider().get_balance(scoped.eoa()).await .map_err(|e| /* ... */)?; health.balance = balance; health.balance_fetched_at = now; scoped.update_health_data(&health).await?; } if health.balance <= health.balance_threshold { tracing::warn!(/* ... */); return Ok(0); }
763-769: Remove commented-out code.This commented code appears to be unused and should be removed to keep the codebase clean.
- // let highest_submitted_nonce_txs = - // scoped.get_highest_submitted_nonce_tranasactions().await?; - - // let highest_submitted_nonce = highest_submitted_nonce_txs - // .first() - // .and_then(|tx| Some(tx.nonce)); -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
executors/src/eoa/mod.rs(1 hunks)executors/src/eoa/store.rs(0 hunks)executors/src/eoa/store/atomic.rs(1 hunks)executors/src/eoa/store/error.rs(1 hunks)executors/src/eoa/store/mod.rs(1 hunks)executors/src/eoa/store/submitted.rs(1 hunks)executors/src/eoa/worker.rs(46 hunks)server/src/execution_router/mod.rs(4 hunks)server/src/main.rs(2 hunks)server/src/queue/manager.rs(2 hunks)
💤 Files with no reviewable changes (1)
- executors/src/eoa/store.rs
✅ Files skipped from review due to trivial changes (1)
- executors/src/eoa/mod.rs
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/error.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
server/src/execution_router/mod.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
server/src/queue/manager.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
executors/src/eoa/store/submitted.rs (1)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
executors/src/eoa/store/atomic.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/mod.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/worker.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
🧬 Code Graph Analysis (1)
executors/src/eoa/store/error.rs (1)
executors/src/eoa/store/atomic.rs (1)
eoa(282-284)
🔇 Additional comments (11)
server/src/main.rs (1)
54-54: Clean architectural refactoring for Redis client management.The centralization of Redis client creation and explicit passing of connection managers improves modularity and makes the dependencies clearer.
Also applies to: 57-57, 78-79
executors/src/eoa/store/error.rs (1)
1-22: Well-structured error helper methods.The error constructors properly encapsulate the context needed for debugging lock loss and nonce synchronization issues, which aligns with the WATCH-based coordination pattern mentioned in the learnings.
server/src/execution_router/mod.rs (1)
413-421: Clean refactoring to local store instantiation.The change from shared store to local instantiation with explicit Redis connection manager improves isolation and makes the store lifecycle clearer.
server/src/queue/manager.rs (1)
50-50: Consistent refactoring for Redis connection management.The changes properly propagate the new Redis client management pattern through the queue layer, maintaining consistency with the broader architectural changes.
Also applies to: 215-216
executors/src/eoa/store/submitted.rs (2)
19-65: Well-implemented transaction serialization with robust error handling.The
SubmittedTransactionstruct properly handles Redis string parsing with appropriate error logging for malformed data. The format documentation is clear and helpful.
216-276: Comprehensive violation detection for data integrity.The
detect_violationsfunction thoroughly identifies anomalies including cross-nonce duplications, multiple confirmations per nonce, and missing receipts. This provides excellent observability for debugging transaction processing issues.executors/src/eoa/store/atomic.rs (3)
19-34: Well-designed trait for atomic Redis operations.The separation of validation and operation phases is excellent, allowing precondition checks before pipeline execution. The trait provides a clean abstraction for WATCH/MULTI/EXEC patterns.
387-464: Robust implementation of lock-protected operations.The method properly implements WATCH/MULTI/EXEC with exponential backoff retry logic. Lock ownership is validated both before and after operations, ensuring consistency.
594-812: Consistent and well-structured atomic operations.All atomic methods follow the same pattern using the
execute_with_watch_and_retryhelper, ensuring consistency and proper error handling across operations.executors/src/eoa/store/mod.rs (1)
590-621: Well-implemented transaction addition with atomic operations.Good use of pipeline to atomically store transaction data and add to pending queue. The timestamp-based ordering in the sorted set allows processing transactions in FIFO order.
executors/src/eoa/worker.rs (1)
254-424: Excellent refactoring to use atomic store.The worker has been successfully refactored to use the new
AtomicEoaExecutorStore. The aggressive lock acquisition and WATCH-based coordination mentioned in the learnings are properly implemented. The error handling and state transitions are well-structured.
| "eoa_{}_{}", | ||
| eoa_execution_options.from, base_execution_options.chain_id | ||
| ), | ||
| noop_signing_credential: signing_credential, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Misleading field name noop_signing_credential.
The field name suggests this is a placeholder or no-operation credential, but it's actually passed as the real signing credential for EOA transactions. Consider renaming to signing_credential or fallback_signing_credential for clarity.
- noop_signing_credential: signing_credential,
+ signing_credential: signing_credential,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| noop_signing_credential: signing_credential, | |
| signing_credential: signing_credential, |
🤖 Prompt for AI Agents
In server/src/execution_router/mod.rs at line 440, the field named
`noop_signing_credential` is misleading because it is used as the actual signing
credential for EOA transactions. Rename this field to a clearer name such as
`signing_credential` or `fallback_signing_credential` to accurately reflect its
purpose and improve code readability.
| /// self.execute_with_watch_and_retry(, worker_id, &safe_tx).await?; | ||
| /// ``` |
There was a problem hiding this comment.
Fix incorrect example in documentation.
The example shows passing worker_id as a parameter, but the method signature doesn't include this parameter.
-/// self.execute_with_watch_and_retry(, worker_id, &safe_tx).await?;
+/// self.execute_with_watch_and_retry(&safe_tx).await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// self.execute_with_watch_and_retry(, worker_id, &safe_tx).await?; | |
| /// ``` | |
| /// self.execute_with_watch_and_retry(&safe_tx).await?; | |
| /// |
🤖 Prompt for AI Agents
In executors/src/eoa/store/atomic.rs around lines 491 to 492, the documentation
example incorrectly shows passing `worker_id` as a parameter to the method
`execute_with_watch_and_retry`, but the method signature does not accept this
parameter. Update the example to remove `worker_id` from the argument list so it
matches the actual method signature.
| /// | ||
| /// This will return all hash:id pairs for the highest nonce | ||
| #[tracing::instrument(skip_all)] | ||
| pub async fn get_highest_submitted_nonce_tranasactions( |
There was a problem hiding this comment.
Fix typo in method name.
The method name contains a typo: "tranasactions" should be "transactions".
- pub async fn get_highest_submitted_nonce_tranasactions(
+ pub async fn get_highest_submitted_nonce_transactions(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn get_highest_submitted_nonce_tranasactions( | |
| pub async fn get_highest_submitted_nonce_transactions( |
🤖 Prompt for AI Agents
In executors/src/eoa/store/mod.rs at line 638, the method name
get_highest_submitted_nonce_tranasactions contains a typo. Rename the method to
get_highest_submitted_nonce_transactions by correcting "tranasactions" to
"transactions" to fix the spelling error.
executors/src/eoa/worker.rs
Outdated
| @@ -1178,7 +1129,7 @@ where | |||
| .map(|(i, prepared)| async move { | |||
| // Add delay for ordering (except first transaction) | |||
| if i > 0 { | |||
| sleep(Duration::from_millis(50)).await; // 50ms delay between consecutive nonces | |||
| sleep(Duration::from_millis(50 * i as u64)).await; // 50ms delay between consecutive nonces | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider capping the send delay to prevent excessive waits.
The linear scaling of delays (50ms * index) could lead to large delays for batches. For 20 transactions, the last one waits 950ms. Consider capping or using a different strategy.
-sleep(Duration::from_millis(50 * i as u64)).await; // 50ms delay between consecutive nonces
+// Cap delay at 200ms to prevent excessive waits
+let delay_ms = (50 * i as u64).min(200);
+sleep(Duration::from_millis(delay_ms)).await;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sleep(Duration::from_millis(50 * i as u64)).await; // 50ms delay between consecutive nonces | |
| // Cap delay at 200ms to prevent excessive waits | |
| let delay_ms = (50 * i as u64).min(200); | |
| sleep(Duration::from_millis(delay_ms)).await; |
🤖 Prompt for AI Agents
In executors/src/eoa/worker.rs at line 1132, the delay before sending
transactions increases linearly with the index, which can cause excessive wait
times for large batches. Modify the code to cap the delay at a maximum value,
for example, use the minimum between 50ms times the index and a fixed upper
limit like 500ms. This prevents the delay from growing indefinitely while
maintaining staggered sends.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
executors/src/eoa/worker.rs (1)
1137-1137: Consider capping the send delay to prevent excessive waits.The linear scaling of delays (50ms * index) could lead to large delays for batches. For 20 transactions, the last one waits 950ms. Consider capping or using a different strategy.
🧹 Nitpick comments (7)
executors/src/webhook/mod.rs (1)
482-482: Remove unnecessary return keyword.The
returnkeyword is not needed here as this is the last expression in the closure.Apply this diff:
- return (payload, webhook_notification_envelope); + (payload, webhook_notification_envelope)executors/src/eoa/events.rs (1)
48-119: Consider removing unnecessary clones for better performance.The methods are well-structured, but several
.clone()calls could be avoided by taking ownership of the parameters instead of borrowing.For example, in
send_attempt_success_envelopes:pub fn send_attempt_success_envelopes( &self, - submitted_transaction: SubmittedTransaction, + submitted_transaction: SubmittedTransaction, ) -> BareWebhookNotificationEnvelope<SerializableSuccessData<SubmittedTransaction>> { BareWebhookNotificationEnvelope { transaction_id: self.transaction_data.transaction_id.clone(), executor_name: EXECUTOR_NAME.to_string(), stage_name: EoaExecutorStage::SendAttempt.to_string(), event_type: StageEvent::Success, payload: SerializableSuccessData { - result: submitted_transaction.clone(), + result: submitted_transaction, }, } }Similar optimizations can be applied to other methods where the parameter is already owned.
executors/src/eoa/store/submitted.rs (3)
89-89: Fix typo in comment."uknown" should be "unknown".
Apply this diff:
- /// Any nonces that have no confirmations (transactions we sent got replaced by a different one uknown to us) + /// Any nonces that have no confirmations (transactions we sent got replaced by a different one unknown to us)
190-191: Address TODO comment for production readiness.The TODO comment indicates missing success job queuing functionality that should be implemented for production readiness.
This appears to be part of the webhook notification system. The success jobs should likely trigger webhook notifications for confirmed transactions. Would you like me to help implement this or create an issue to track this requirement?
22-48: Consider improving error handling in parsing logic.The
from_redis_stringsmethod filters out invalid entries but only logs errors. For production readiness, consider whether parsing failures should be handled more explicitly or if additional validation is needed.Consider adding metrics or more structured error handling:
} else { - tracing::error!("Invalid queued_at timestamp: {}", tx.0); + tracing::error!( + redis_string = %tx.0, + "Invalid queued_at timestamp format in submitted transaction" + ); + // Consider adding metrics here for monitoring None }executors/src/eoa/worker.rs (2)
497-497: Address the batching TODO for performance optimization.The sequential processing of borrowed state transitions could become a bottleneck under high load. Consider implementing batch operations for better performance.
Would you like me to help design a batched approach for these atomic operations?
768-773: Remove commented code or implement the functionality.The commented code for highest submitted nonce transactions should either be removed if not needed or properly implemented if required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
executors/src/eoa/events.rs(1 hunks)executors/src/eoa/mod.rs(1 hunks)executors/src/eoa/store/submitted.rs(1 hunks)executors/src/eoa/worker.rs(46 hunks)executors/src/webhook/envelope.rs(1 hunks)executors/src/webhook/mod.rs(3 hunks)server/src/queue/manager.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- executors/src/eoa/mod.rs
- server/src/queue/manager.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/events.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/submitted.rs (1)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
executors/src/eoa/worker.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
🔇 Additional comments (12)
executors/src/webhook/envelope.rs (1)
44-71: Well-designed webhook envelope pattern.The separation between
BareWebhookNotificationEnvelopeandWebhookNotificationEnvelopeis a clean design pattern that allows for composable webhook notification creation. The conversion method appropriately generates metadata like notification IDs while preserving the core notification data.executors/src/webhook/mod.rs (1)
435-507: Well-implemented webhook queuing function.The function properly handles serialization, job creation, and logging. The error handling with
Resulttypes is appropriate, and the use of structured logging provides good observability.executors/src/eoa/events.rs (1)
35-44: Clean enum design with proper Display implementation.The
EoaExecutorStageenum with itsDisplayimplementation follows Rust conventions well and provides clear string representations for the different stages.executors/src/eoa/store/submitted.rs (1)
107-216: Well-implemented atomic transaction pattern.The
SafeRedisTransactionimplementation properly handles Redis WATCH operations, validation, and atomic updates. The cleanup logic correctly reconciles submitted transactions with confirmed ones while detecting various violation scenarios. This follows the learnings about WATCH-based coordination for safe Redis operations.executors/src/eoa/worker.rs (8)
14-39: LGTM! Import changes align with architectural improvements.The new imports for Redis connection management, webhook functionality, and atomic store operations support the transition to direct Redis management and improved concurrency control.
97-101: Excellent error classification improvement.Adding a dedicated
TransactionSendErrorvariant provides better distinction between transaction broadcast failures and generic RPC errors, which will improve debugging and error handling logic.
241-248: Good enhancement with rich receipt data.Including the full
TransactionReceiptprovides comprehensive transaction information for confirmed transactions, which is valuable for downstream processing and debugging.
279-283: Approve architectural changes to worker structure.The shift to direct Redis connection management and webhook queue integration aligns with the new atomic store design and improves worker autonomy.
505-530: Excellent use of atomic operations for crash recovery.The atomic state transitions from borrowed to submitted/recycled with proper error classification ensure data consistency during crash recovery scenarios.
1795-1836: Approve the improved credential handling.Making
SigningCredentialan explicit parameter improves method clarity and allows better separation of signing logic from transaction data management.
630-685: Excellent improvements to confirmation flow processing.The new approach with
CleanupReport, rich receipt data collection, and proper transaction categorization provides much better visibility into transaction processing outcomes.
432-449: Good implementation of explicit lock management.The direct Redis
DELoperation for lock release with proper error logging provides clear lock lifecycle management and good visibility into potential issues.
executors/src/eoa/worker.rs
Outdated
| // TODO: refactor this, very ugly | ||
| if health.balance <= health.balance_threshold { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the health checking logic.
The TODO comment indicates this logic needs cleanup. The nested conditionals and repeated balance fetching make the code hard to follow.
Consider extracting the balance checking logic into a separate method:
- // TODO: refactor this, very ugly
- if health.balance <= health.balance_threshold {
- if now - health.balance_fetched_at > HEALTH_CHECK_INTERVAL {
- let balance = chain
- .provider()
- .get_balance(scoped.eoa())
- .await
- .map_err(|e| {
- let engine_error = e.to_engine_error(chain);
- EoaExecutorWorkerError::RpcError {
- message: format!("Failed to get balance: {}", engine_error),
- inner_error: engine_error,
- }
- })?;
-
- health.balance = balance;
- health.balance_fetched_at = now;
- scoped.update_health_data(&health).await?;
- }
-
- if health.balance <= health.balance_threshold {
- tracing::warn!(
- "EOA has insufficient balance (<= {} wei), skipping send flow",
- health.balance_threshold
- );
- return Ok(0);
- }
- }
+ if !self.ensure_sufficient_balance(&mut health, scoped, chain, now).await? {
+ return Ok(0);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In executors/src/eoa/worker.rs around lines 699 to 700, the health checking
logic is cluttered and hard to read due to nested conditionals and repeated
balance checks. Refactor by extracting the balance comparison into a dedicated
method that returns a boolean indicating if the balance is below or equal to the
threshold. Replace the existing inline conditional with a call to this new
method to improve readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
executors/src/eoa/events.rs (1)
78-82: Consider making retry configuration more explicit.The hardcoded values in the nack envelope (delay_ms: None, max_attempts: None, etc.) may be intentional design decisions, but consider whether these should be configurable parameters for different deployment environments.
If retry behavior needs to be configurable, consider adding a configuration parameter:
pub fn send_attempt_nack_envelope( &self, nonce: u64, error: EoaExecutorWorkerError, attempt_number: u32, + retry_config: Option<&RetryConfig>, ) -> BareWebhookNotificationEnvelope<SerializableNackData<EoaSendAttemptNackData>>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
executors/src/eoa/events.rs(1 hunks)executors/src/webhook/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- executors/src/webhook/mod.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/events.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
🔇 Additional comments (6)
executors/src/eoa/events.rs (6)
1-15: Well-organized imports with clear module separation.The imports are logically grouped and provide clear access to the necessary dependencies for event handling and webhook notifications.
17-19: Simple and focused event structure.The
EoaExecutorEventstruct provides a clean wrapper around transaction data for event generation.
21-25: Comprehensive nack data structure with proper serialization support.The struct correctly captures both the nonce and error details needed for nack events, with appropriate derives for the webhook system.
27-42: Well-defined stage enum with consistent string representation.The
EoaExecutorStageenum properly covers the key transaction lifecycle stages, and theDisplayimplementation uses consistent snake_case naming for external APIs.
46-134: Consistent and well-structured envelope creation methods.All envelope creation methods follow a consistent pattern and properly populate the webhook notification structure. The use of cloning is appropriate for the webhook system's needs.
118-133: Stage mapping is correct as implemented.The
EoaExecutorStageenum only defines three variants—SendAttempt,TransactionReplaced, andTransactionConfirmed—and the failure envelope appropriately usesSendAttempt. There is no dedicated “failure” stage in the enum, so mapping failures to the send‐attempt stage is intentional and consistent with the other envelope methods:
transaction_replaced_envelope→TransactionReplacedtransaction_confirmed_envelope→TransactionConfirmedtransaction_failed_envelope→SendAttemptNo changes required here.
- Introduced `transaction_context_from_pipeline` method to allow atomic job queuing within an existing Redis transaction. - This enhancement improves the flexibility of transaction handling in the queue implementation.
- Added `thirdweb-core` as a dependency in `Cargo.toml` and updated `Cargo.lock`. - Refactored transaction handling in the EOA executor to improve error handling and processing of borrowed transactions. - Introduced new methods for moving pending transactions to borrowed state using incremented and recycled nonces. - Enhanced submission result processing to include detailed reporting and webhook event queuing for transaction outcomes. These changes aim to improve the robustness and flexibility of transaction management within the EOA executor.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
executors/src/eoa/store/atomic.rs (1)
269-270: Fix incorrect example in documentation.The example shows passing
worker_idas a parameter, but the method signature doesn't include this parameter.-/// self.execute_with_watch_and_retry(, worker_id, &safe_tx).await?; +/// self.execute_with_watch_and_retry(&safe_tx).await?;executors/src/eoa/store/mod.rs (1)
678-678: Fix typo in method name.The method name contains a typo: "tranasactions" should be "transactions".
- pub async fn get_highest_submitted_nonce_tranasactions( + pub async fn get_highest_submitted_nonce_transactions(
🧹 Nitpick comments (4)
executors/src/eoa/store/pending.rs (1)
187-198: Consider using batched operations for recycled nonce validation.While the current implementation is correct, you could optimize the recycled nonce validation by using a pipeline to check existence, similar to how you batch ZSCORE operations for pending transactions.
- // Get all recycled nonces - let recycled_nonces: HashSet<u64> = conn - .zrange(self.keys.recycled_nonces_zset_name(), 0, -1) - .await?; - - // Verify all nonces are in recycled set - for tx in self.transactions { - let nonce = tx.signed_transaction.nonce(); - if !recycled_nonces.contains(&nonce) { - return Err(TransactionStoreError::NonceNotInRecycledSet { nonce }); - } - } + // Verify all nonces exist in recycled set using batched ZSCORE + let mut pipe = twmq::redis::pipe(); + for tx in self.transactions { + let nonce = tx.signed_transaction.nonce(); + pipe.zscore(self.keys.recycled_nonces_zset_name(), nonce); + } + let scores: Vec<Option<f64>> = pipe.query_async(conn).await?; + + for (tx, score) in self.transactions.iter().zip(scores.iter()) { + if score.is_none() { + let nonce = tx.signed_transaction.nonce(); + return Err(TransactionStoreError::NonceNotInRecycledSet { nonce }); + } + }This approach avoids loading all recycled nonces into memory when you only need to check specific ones.
executors/src/eoa/store/atomic.rs (1)
552-555: Incomplete webhook implementation for failed transactions.The TODO comment indicates missing webhook functionality. This should be implemented to ensure proper notification of transaction failures.
Would you like me to implement the webhook queuing for failed pending transactions? I can generate the code to properly queue the webhook event with the failure details.
executors/src/eoa/worker.rs (2)
511-529: Consider adding retry logic for lock releaseThe lock release implementation is correct, but consider adding retry logic for transient Redis failures to ensure locks are properly released.
async fn release_eoa_lock(&self, job_data: &EoaExecutorWorkerJobData) { let keys = EoaExecutorStoreKeys::new( job_data.eoa_address, job_data.chain_id, self.namespace.clone(), ); let lock_key = keys.eoa_lock_key_name(); let mut conn = self.redis.clone(); - if let Err(e) = conn.del::<&str, ()>(&lock_key).await { - tracing::error!( - eoa = %job_data.eoa_address, - chain_id = %job_data.chain_id, - error = %e, - "Failed to release EOA lock" - ); + + // Retry up to 3 times with exponential backoff + for attempt in 0..3 { + match conn.del::<&str, ()>(&lock_key).await { + Ok(_) => return, + Err(e) if attempt < 2 => { + tracing::warn!( + eoa = %job_data.eoa_address, + chain_id = %job_data.chain_id, + error = %e, + attempt = attempt + 1, + "Failed to release EOA lock, retrying" + ); + sleep(Duration::from_millis(100 * 2_u64.pow(attempt))).await; + } + Err(e) => { + tracing::error!( + eoa = %job_data.eoa_address, + chain_id = %job_data.chain_id, + error = %e, + "Failed to release EOA lock after retries" + ); + return; + } + } } }
393-440: Well-designed helper methods for submission result handlingThe
SubmissionResulthelper methods properly encapsulate error classification and result creation logic. Consider adding unit tests to ensure the error classification behaves correctly for various RPC error scenarios.Would you like me to generate unit tests for these helper methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
executors/Cargo.toml(1 hunks)executors/src/eoa/store/atomic.rs(1 hunks)executors/src/eoa/store/borrowed.rs(1 hunks)executors/src/eoa/store/mod.rs(1 hunks)executors/src/eoa/store/pending.rs(1 hunks)executors/src/eoa/store/submitted.rs(1 hunks)executors/src/eoa/worker.rs(33 hunks)
✅ Files skipped from review due to trivial changes (1)
- executors/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- executors/src/eoa/store/submitted.rs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/pending.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/borrowed.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/atomic.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/worker.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
executors/src/eoa/store/mod.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (5)
executors/src/eoa/store/pending.rs (1)
11-145: Well-implemented atomic operation for incremented nonce allocation.The implementation correctly:
- Validates sequential nonces without gaps
- Uses batched ZSCORE operations for efficient pending queue validation
- Pre-serializes data to avoid failures during pipeline execution
- Atomically updates all relevant Redis structures
executors/src/eoa/worker.rs (4)
1180-1232: Well-implemented retry logic for transaction preparationThe retry mechanism with exponential backoff and proper error classification is a great addition. It appropriately distinguishes between retryable network errors and deterministic failures.
598-600: Efficient batch processing with atomic guaranteesThe move to batch processing via
process_borrowed_transactionsis a good optimization. Based on the learnings about WATCH-based coordination in the Redis store, this maintains atomicity while improving performance.Also applies to: 977-979, 1155-1157
1770-1771: Good security improvement with explicit credential handlingThe explicit passing of
fromaddress andsigning_credentialparameters improves security by making credential usage transparent and reducing the risk of signing with wrong credentials.Also applies to: 1803-1808
1263-1302: LGTM!The
send_noop_transactionimplementation correctly handles noop transactions for recycled nonces with proper error wrapping.
executors/src/eoa/store/borrowed.rs
Outdated
| // We'll set attempt_number to 1 for simplicity in the operation phase | ||
| // The actual attempt tracking is handled by the attempts list | ||
| let attempt_number = 1; | ||
|
|
There was a problem hiding this comment.
Incorrect attempt number hardcoded to 1.
The attempt number is hardcoded to 1, but this doesn't reflect the actual number of attempts. You should calculate the correct attempt number based on the existing attempts.
- // We'll set attempt_number to 1 for simplicity in the operation phase
- // The actual attempt tracking is handled by the attempts list
- let attempt_number = 1;
+ // Calculate the actual attempt number
+ let attempts_key = self.keys.transaction_attempts_list_name(transaction_id);
+ // Since we're about to add a new attempt, the current count will be the attempt number
+ let attempt_number = borrowed_tx.borrowed_at; // This seems wrong, need to get actual countActually, you'll need to get the attempt count from the validation phase or calculate it differently. Consider passing the attempt counts from validation to operation phase.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In executors/src/eoa/store/borrowed.rs around lines 229 to 232, the
attempt_number is incorrectly hardcoded to 1, which does not reflect the actual
number of attempts. To fix this, calculate the attempt_number based on the
length of the existing attempts list or pass the attempt count from the
validation phase to the operation phase, ensuring the attempt_number accurately
represents the current attempt count.
executors/src/eoa/store/borrowed.rs
Outdated
| let borrowed_transactions: Vec<BorrowedTransactionData> = borrowed_transactions_map | ||
| .into_iter() | ||
| .filter_map(|(nonce_str, data_json)| { | ||
| let borrowed_data: BorrowedTransactionData = | ||
| serde_json::from_str(&data_json).ok()?; | ||
| Some(borrowed_data) | ||
| }) | ||
| .collect(); | ||
|
|
There was a problem hiding this comment.
Handle deserialization errors explicitly instead of silently ignoring them.
The current implementation silently ignores deserialization errors, which could lead to transactions being lost without proper error reporting.
let borrowed_transactions: Vec<BorrowedTransactionData> = borrowed_transactions_map
.into_iter()
- .filter_map(|(nonce_str, data_json)| {
- let borrowed_data: BorrowedTransactionData =
- serde_json::from_str(&data_json).ok()?;
- Some(borrowed_data)
- })
+ .map(|(nonce_str, data_json)| {
+ serde_json::from_str::<BorrowedTransactionData>(&data_json)
+ .map_err(|e| TransactionStoreError::DeserError {
+ message: format!("Failed to deserialize borrowed transaction for nonce {}: {}", nonce_str, e),
+ text: data_json,
+ })
+ })
+ .collect::<Result<Vec<_>, _>>()?;
- .collect();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let borrowed_transactions: Vec<BorrowedTransactionData> = borrowed_transactions_map | |
| .into_iter() | |
| .filter_map(|(nonce_str, data_json)| { | |
| let borrowed_data: BorrowedTransactionData = | |
| serde_json::from_str(&data_json).ok()?; | |
| Some(borrowed_data) | |
| }) | |
| .collect(); | |
| let borrowed_transactions: Vec<BorrowedTransactionData> = borrowed_transactions_map | |
| .into_iter() | |
| .map(|(nonce_str, data_json)| { | |
| serde_json::from_str::<BorrowedTransactionData>(&data_json) | |
| .map_err(|e| TransactionStoreError::DeserError { | |
| message: format!( | |
| "Failed to deserialize borrowed transaction for nonce {}: {}", | |
| nonce_str, e | |
| ), | |
| text: data_json, | |
| }) | |
| }) | |
| .collect::<Result<Vec<_>, _>>()?; |
🤖 Prompt for AI Agents
In executors/src/eoa/store/borrowed.rs around lines 145 to 153, the code
currently uses filter_map with serde_json::from_str().ok() to deserialize JSON,
which silently ignores errors. Modify this to explicitly handle deserialization
errors by matching the result of from_str and logging or returning an error when
deserialization fails, instead of skipping the entry silently. This will ensure
that deserialization issues are reported and not lost.
| // todo: in case of non-existent created_at, we should return a default value | ||
| let created_at = | ||
| created_at | ||
| .parse::<u64>() | ||
| .map_err(|_| TransactionStoreError::TransactionNotFound { | ||
| transaction_id: transaction_id.to_string(), | ||
| })?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for missing or invalid created_at field.
The current implementation fails completely if created_at is missing or invalid. As the TODO suggests, consider using a default value for backward compatibility.
- // todo: in case of non-existent created_at, we should return a default value
- let created_at =
- created_at
- .parse::<u64>()
- .map_err(|_| TransactionStoreError::TransactionNotFound {
- transaction_id: transaction_id.to_string(),
- })?;
+ // Use a default value (0) for backward compatibility if created_at is missing or invalid
+ let created_at = created_at
+ .parse::<u64>()
+ .unwrap_or_else(|_| {
+ tracing::warn!(
+ transaction_id = %transaction_id,
+ "Missing or invalid created_at field, using default value 0"
+ );
+ 0
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // todo: in case of non-existent created_at, we should return a default value | |
| let created_at = | |
| created_at | |
| .parse::<u64>() | |
| .map_err(|_| TransactionStoreError::TransactionNotFound { | |
| transaction_id: transaction_id.to_string(), | |
| })?; | |
| // Use a default value (0) for backward compatibility if created_at is missing or invalid | |
| let created_at = created_at | |
| .parse::<u64>() | |
| .unwrap_or_else(|_| { | |
| tracing::warn!( | |
| transaction_id = %transaction_id, | |
| "Missing or invalid created_at field, using default value 0" | |
| ); | |
| 0 | |
| }); |
🤖 Prompt for AI Agents
In executors/src/eoa/store/mod.rs around lines 591 to 597, the code currently
returns an error if the created_at field is missing or invalid. To improve error
handling and maintain backward compatibility, modify the code to check if
created_at exists and is valid; if not, assign a sensible default value instead
of returning an error. This ensures the function continues gracefully when
created_at is absent or malformed.
executors/src/eoa/worker.rs
Outdated
| // TODO: Implement post-processing analysis for balance threshold updates and nonce resets | ||
| // Currently we lose the granular error handling that was in the individual atomic operations. | ||
| // Consider: | ||
| // 1. Analyzing submission_results for specific error patterns | ||
| // 2. Calling update_balance_threshold if needed | ||
| // 3. Detecting nonce reset conditions | ||
| // 4. Or move this logic into the batch processor itself | ||
|
|
||
| // Process all results in one batch operation | ||
| let report = scoped | ||
| .process_borrowed_transactions(submission_results, self.webhook_queue.clone()) | ||
| .await?; | ||
|
|
||
| // Check if this should trigger nonce reset | ||
| if should_trigger_nonce_reset(&e) { | ||
| tracing::warn!( | ||
| eoa = %scoped.eoa(), | ||
| chain_id = %scoped.chain_id(), | ||
| "Nonce too high error detected, may need nonce synchronization" | ||
| ); | ||
| // The next confirm_flow will fetch fresh nonce and auto-sync | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // TODO: Handle post-processing updates here if needed | ||
| // For now, we skip the individual error analysis that was done in the old atomic approach | ||
|
|
There was a problem hiding this comment.
Critical: Missing post-processing for balance threshold updates and nonce resets
The crash recovery flow loses important error analysis that was previously done in individual atomic operations. This could lead to:
- Missed balance threshold updates when transactions fail due to insufficient funds
- Failure to detect and handle nonce reset conditions
- Lost insights into specific error patterns
Consider implementing the post-processing analysis as suggested in the TODO comments, or move this logic into the batch processor itself.
🤖 Prompt for AI Agents
In executors/src/eoa/worker.rs around lines 589 to 604, the current code lacks
post-processing analysis for balance threshold updates and nonce resets after
processing transaction results. To fix this, implement logic that inspects
submission_results for specific error patterns, calls update_balance_threshold
when insufficient funds errors are detected, and identifies nonce reset
conditions to handle them appropriately. Alternatively, integrate this error
analysis and handling directly into the batch processor to restore the granular
error handling lost from the previous atomic operations.
executors/src/eoa/worker.rs
Outdated
| // Track balance threshold issues | ||
| if let EoaExecutorWorkerError::TransactionSimulationFailed { | ||
| inner_error, | ||
| .. | ||
| } = &e | ||
| { | ||
| if should_update_balance_threshold(inner_error) { | ||
| balance_threshold_update_needed = true; | ||
| } | ||
| } else if let EoaExecutorWorkerError::RpcError { inner_error, .. } = &e { | ||
| if should_update_balance_threshold(inner_error) { | ||
| balance_threshold_update_needed = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicate balance threshold checking logic
This balance threshold checking logic is duplicated between process_recycled_nonces and process_new_transactions.
Extract this into a helper method:
+ /// Check if error indicates balance issues and update threshold if needed
+ async fn handle_balance_threshold_error(
+ &self,
+ error: &EoaExecutorWorkerError,
+ scoped: &AtomicEoaExecutorStore,
+ chain: &impl Chain,
+ ) -> bool {
+ let needs_update = match error {
+ EoaExecutorWorkerError::TransactionSimulationFailed { inner_error, .. } => {
+ should_update_balance_threshold(inner_error)
+ }
+ EoaExecutorWorkerError::RpcError { inner_error, .. } => {
+ should_update_balance_threshold(inner_error)
+ }
+ _ => false,
+ };
+
+ if needs_update {
+ if let Err(e) = self.update_balance_threshold(scoped, chain).await {
+ tracing::error!("Failed to update balance threshold: {}", e);
+ }
+ }
+ needs_update
+ }Then use it in both places:
- // Track balance threshold issues
- if let EoaExecutorWorkerError::TransactionSimulationFailed {
- inner_error,
- ..
- } = &e
- {
- if should_update_balance_threshold(inner_error) {
- balance_threshold_update_needed = true;
- }
- } else if let EoaExecutorWorkerError::RpcError { inner_error, .. } = &e {
- if should_update_balance_threshold(inner_error) {
- balance_threshold_update_needed = true;
- }
- }
+ if self.handle_balance_threshold_error(&e, scoped, chain).await {
+ balance_threshold_update_needed = true;
+ }Also applies to: 1054-1067
🤖 Prompt for AI Agents
In executors/src/eoa/worker.rs around lines 872 to 885 and also 1054 to 1067,
the balance threshold checking logic is duplicated. Extract this logic into a
separate helper function that takes the error as input and returns whether the
balance threshold update is needed. Replace the duplicated code in both
process_recycled_nonces and process_new_transactions with calls to this new
helper function to improve code reuse and maintainability.
- Updated the `SmartAccountSigner` to pass credentials by reference instead of cloning, enhancing performance. - Modified transaction structures to include default serialization options for gas limits and transaction type data. - Enhanced webhook options handling in various job data structures, ensuring consistent default behavior. - Refactored the EOA executor to streamline transaction confirmation and submission processes, improving overall transaction management. These changes aim to optimize resource usage and improve the clarity of transaction handling across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
core/src/signer.rs (1)
172-198: Update call sites to pass references, not owned valuesThe following invocations in core/src/signer.rs still use
.clone()or owned parameters; they need to pass borrowed data to match the new signatures:
- Line 289
Current:Change to:.sign_typed_data(auth_method.clone(), typed_data.clone(), options.from)-.sign_typed_data(auth_method.clone(), typed_data.clone(), options.from)
+.sign_typed_data(auth_method.clone(), &typed_data, &options.from)- **Line 304** Current: ```rust .sign_typed_data(auth_token, thirdweb_auth, typed_data, options.from)Change to:
-.sign_typed_data(auth_token, thirdweb_auth, typed_data, options.from) +.sign_typed_data(auth_token, thirdweb_auth, &typed_data, &options.from)
- Line 326
Current:Change to:.sign_transaction(auth_method.clone(), transaction.clone(), options.from)-.sign_transaction(auth_method.clone(), transaction.clone(), options.from)
+.sign_transaction(auth_method.clone(), &transaction, &options.from)- **Line 371** Current: ```rust .sign_authorization(auth_method.clone(), options.from, authorization)Change to:
-.sign_authorization(auth_method.clone(), options.from, authorization) +.sign_authorization(auth_method.clone(), &options.from, &authorization)All other callers already pass references for
TypedData,TypedTransaction, andSigningCredential. Please apply these fixes so the code compiles against the updated trait.
♻️ Duplicate comments (4)
executors/src/eoa/store/borrowed.rs (1)
181-181: Incorrect attempt number hardcoded to 1.The attempt number is hardcoded to 1 in both nack and fail cases, which doesn't reflect the actual number of attempts.
Also applies to: 212-212
executors/src/eoa/store/atomic.rs (1)
275-276: Fix incorrect example in documentation.The example shows passing
worker_idas a parameter, but the method signature doesn't include this parameter.executors/src/eoa/store/mod.rs (2)
720-720: Fix typo in method name.The method name contains a typo: "tranasactions" should be "transactions".
633-639: Improve error handling for missing or invalid created_at field.The current implementation fails completely if
created_atis missing or invalid. As the TODO suggests, consider using a default value for backward compatibility.
🧹 Nitpick comments (10)
server/src/http/routes/admin/queue.rs (1)
32-32: Consider extracting queue names to avoid duplication.The queue names are duplicated in the API documentation (line 32) and error message (lines 91-92). Consider defining a constant array or enum to maintain this list in one place.
+const VALID_QUEUE_NAMES: &[&str] = &[ + "webhook", + "external_bundler_send", + "userop_confirm", + "eoa_executor", + "eip7702_send", + "eip7702_confirm", +]; #[utoipa::path( post, operation_id = "emptyQueueIdempotencySet", path = "/admin/queue/{queue_name}/empty-idempotency-set", tag = "Admin", responses( (status = 200, description = "Successfully emptied idempotency set", body = SuccessResponse<EmptyIdempotencySetResponse>, content_type = "application/json"), ), params( - ("queue_name" = String, Path, description = "Queue name - one of: webhook, external_bundler_send, userop_confirm, eoa_executor, eip7702_send, eip7702_confirm"), + ("queue_name" = String, Path, description = format!("Queue name - one of: {}", VALID_QUEUE_NAMES.join(", "))), ) )]And in the error message:
engine_core::error::EngineError::ValidationError { message: format!( - "Invalid queue name '{}'. Valid options are: webhook, external_bundler_send, userop_confirm, eoa_executor, eip7702_send, eip7702_confirm", + "Invalid queue name '{}'. Valid options are: {}", - queue_name + queue_name, + VALID_QUEUE_NAMES.join(", ") ), },Also applies to: 50-96
executors/src/eoa/store/pending.rs (1)
189-200: Consider optimizing recycled nonce validation.The current implementation fetches all recycled nonces and then checks membership. For large sets, consider using ZSCORE to check individual nonces instead of fetching the entire set.
-// Get all recycled nonces -let recycled_nonces: HashSet<u64> = conn - .zrange(self.keys.recycled_nonces_zset_name(), 0, -1) - .await?; - -// Verify all nonces are in recycled set -for tx in self.transactions { - let nonce = tx.signed_transaction.nonce(); - if !recycled_nonces.contains(&nonce) { - return Err(TransactionStoreError::NonceNotInRecycledSet { nonce }); - } -} +// Verify all nonces exist in recycled set using batched ZSCORE +let mut pipe = twmq::redis::pipe(); +let nonces: Vec<u64> = self.transactions.iter() + .map(|tx| tx.signed_transaction.nonce()) + .collect(); + +for nonce in &nonces { + pipe.zscore(self.keys.recycled_nonces_zset_name(), nonce); +} + +let scores: Vec<Option<f64>> = pipe.query_async(conn).await?; + +for (nonce, score) in nonces.iter().zip(scores.iter()) { + if score.is_none() { + return Err(TransactionStoreError::NonceNotInRecycledSet { nonce: *nonce }); + } +}executors/src/eoa/worker/send.rs (2)
83-83: Consider making the iteration limit configurable and clarifying no-op transaction logic.
- The hard-coded iteration limit of 10 (line 83) seems arbitrary. Consider making it configurable or documenting why 10 is chosen.
- The no-op transaction logic for leftover recycled nonces could benefit from additional comments explaining the purpose.
+const MAX_RECYCLED_NONCE_ITERATIONS: usize = 10; // Prevent infinite loops while retrying failed preparations -for _ in 0..10 { +for _ in 0..MAX_RECYCLED_NONCE_ITERATIONS {And add a comment for the no-op section:
if is_pending_empty { + // Send no-op transactions for remaining recycled nonces to prevent them from being stuck + // This ensures nonce continuity even when there are no pending transactions let recycled_nonces = self.store.clean_and_get_recycled_nonces().await?;Also applies to: 195-212
276-276: Consider using the same configurable constant for iteration limits.For consistency with
process_recycled_nonces, consider using the same configurable constant for the iteration limit.+// Use the same constant as in process_recycled_nonces -for iteration in 0..10 { +for iteration in 0..MAX_RECYCLED_NONCE_ITERATIONS {executors/src/eoa/worker/transaction.rs (1)
207-225: Consider more explicit EIP7702 transaction detection.The code checks
tx.authorization_list().is_none()to determine if fallback to legacy gas price is allowed. This indirect check could be made more explicit.Consider adding a helper method or using the transaction type directly:
-if tx.authorization_list().is_none() { +// Explicitly check if this is not an EIP7702 transaction +if !matches!(tx.transaction_type(), Some(TransactionType::Eip7702)) {executors/src/eoa/worker/confirm.rs (1)
304-304: Consider making the gas bump percentage configurable.The 20% gas increase is hardcoded. Consider making this configurable to allow for different gas bump strategies based on network conditions.
- let bumped_typed_tx = self.apply_gas_bump_to_typed_transaction(typed_tx, 120); // 20% increase + // TODO: Make gas bump percentage configurable + const GAS_BUMP_PERCENTAGE: u64 = 120; // 20% increase + let bumped_typed_tx = self.apply_gas_bump_to_typed_transaction(typed_tx, GAS_BUMP_PERCENTAGE);executors/src/eoa/store/borrowed.rs (1)
113-116: Address the TODO for attempt tracking.The commented code indicates incomplete attempt tracking logic. This should be implemented to maintain accurate attempt history.
Would you like me to help implement the attempt tracking logic or open an issue to track this task?
executors/src/eoa/worker/mod.rs (1)
403-409: Implement post-processing analysis for error patterns.The TODO comments indicate missing logic for analyzing submission results to detect balance threshold updates and nonce reset conditions.
Would you like me to help implement the post-processing analysis logic to handle balance threshold updates and nonce reset detection based on the submission results?
executors/src/eoa/store/submitted.rs (1)
292-292: Fix incorrect webhook error messages.The error messages incorrectly say "Failed to queue webhook for fail" in both success and replacement contexts.
- tracing::error!("Failed to queue webhook for fail: {}", e); + tracing::error!("Failed to queue webhook for success: {}", e);and
- tracing::error!("Failed to queue webhook for fail: {}", e); + tracing::error!("Failed to queue webhook for replacement: {}", e);Also applies to: 323-323
executors/src/eoa/store/mod.rs (1)
230-234: Fix typo in comment.There's a typo in the comment: "successfuly" should be "successfully".
- /// We store the nonce of the last successfuly sent transaction for each EOA. + /// We store the nonce of the last successfully sent transaction for each EOA.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
aa-core/src/signer.rs(4 hunks)core/src/execution_options/mod.rs(1 hunks)core/src/signer.rs(11 hunks)core/src/transaction.rs(5 hunks)executors/src/eip7702_executor/confirm.rs(1 hunks)executors/src/eip7702_executor/send.rs(3 hunks)executors/src/eoa/events.rs(1 hunks)executors/src/eoa/mod.rs(1 hunks)executors/src/eoa/store/atomic.rs(1 hunks)executors/src/eoa/store/borrowed.rs(1 hunks)executors/src/eoa/store/hydrate.rs(1 hunks)executors/src/eoa/store/mod.rs(1 hunks)executors/src/eoa/store/pending.rs(1 hunks)executors/src/eoa/store/submitted.rs(1 hunks)executors/src/eoa/worker.rs(0 hunks)executors/src/eoa/worker/confirm.rs(1 hunks)executors/src/eoa/worker/error.rs(1 hunks)executors/src/eoa/worker/mod.rs(1 hunks)executors/src/eoa/worker/send.rs(1 hunks)executors/src/eoa/worker/transaction.rs(1 hunks)executors/src/external_bundler/confirm.rs(2 hunks)executors/src/external_bundler/send.rs(3 hunks)executors/src/webhook/envelope.rs(5 hunks)server/src/execution_router/mod.rs(10 hunks)server/src/http/routes/admin/mod.rs(1 hunks)server/src/http/routes/admin/queue.rs(1 hunks)server/src/http/routes/contract_write.rs(1 hunks)server/src/http/routes/mod.rs(1 hunks)server/src/http/routes/sign_message.rs(1 hunks)server/src/http/routes/sign_typed_data.rs(1 hunks)server/src/http/server.rs(1 hunks)server/src/queue/manager.rs(4 hunks)thirdweb-core/src/iaw/mod.rs(4 hunks)twmq/src/lib.rs(4 hunks)
💤 Files with no reviewable changes (1)
- executors/src/eoa/worker.rs
✅ Files skipped from review due to trivial changes (2)
- server/src/http/routes/admin/mod.rs
- core/src/transaction.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- executors/src/eoa/mod.rs
- twmq/src/lib.rs
- executors/src/webhook/envelope.rs
- server/src/execution_router/mod.rs
- server/src/queue/manager.rs
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/worker/send.rs (1)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/worker/confirm.rs (1)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/hydrate.rs (1)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
executors/src/eoa/store/pending.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/submitted.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/events.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
executors/src/eoa/store/borrowed.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/atomic.rs (3)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#14
File: executors/src/eoa/store/atomic.rs:38-42
Timestamp: 2025-07-13T20:57:25.741Z
Learning: As of Rust 1.75 (released December 2023), you can use `impl Trait` as the return type of trait methods, including `impl Future` for async-like behavior without requiring the async-trait crate. This eliminates the need for async-trait in many cases where traits need async-like methods.
executors/src/eoa/worker/mod.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
executors/src/eoa/store/mod.rs (3)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#14
File: executors/src/webhook/mod.rs:439-439
Timestamp: 2025-07-12T01:46:22.885Z
Learning: The user prefers defensive programming approaches to prevent overflow issues when casting between integer types, specifically when converting i64 timestamps to u64 to avoid wraparound from negative values.
🧬 Code Graph Analysis (9)
server/src/http/server.rs (1)
server/src/http/routes/admin/queue.rs (1)
empty_queue_idempotency_set(40-135)
executors/src/eip7702_executor/confirm.rs (4)
executors/src/eip7702_executor/send.rs (1)
webhook_options(56-58)executors/src/external_bundler/confirm.rs (1)
webhook_options(341-343)executors/src/webhook/envelope.rs (1)
webhook_options(107-107)executors/src/external_bundler/send.rs (1)
webhook_options(63-65)
executors/src/external_bundler/confirm.rs (2)
executors/src/webhook/envelope.rs (1)
webhook_options(107-107)executors/src/webhook/mod.rs (1)
webhook_options(441-457)
executors/src/eip7702_executor/send.rs (4)
executors/src/eip7702_executor/confirm.rs (1)
webhook_options(40-42)executors/src/external_bundler/confirm.rs (1)
webhook_options(341-343)executors/src/webhook/envelope.rs (1)
webhook_options(107-107)executors/src/external_bundler/send.rs (1)
webhook_options(63-65)
executors/src/external_bundler/send.rs (2)
executors/src/webhook/envelope.rs (1)
webhook_options(107-107)executors/src/webhook/mod.rs (1)
webhook_options(441-457)
executors/src/eoa/worker/confirm.rs (3)
executors/src/eoa/worker/error.rs (1)
should_update_balance_threshold(165-184)executors/src/eoa/store/mod.rs (3)
nonce(368-373)new(112-118)new(280-294)executors/src/eoa/store/submitted.rs (3)
transaction_id(73-78)hash(59-64)nonce(66-71)
executors/src/eoa/store/submitted.rs (5)
executors/src/eoa/store/atomic.rs (7)
eoa(82-84)name(37-37)watch_keys(48-48)validation(43-47)operation(38-42)pipeline(231-232)pipeline(349-350)executors/src/webhook/mod.rs (1)
queue_webhook_envelopes(433-505)executors/src/eoa/store/mod.rs (7)
nonce(368-373)submitted_transactions_zset_name(171-179)pipeline(700-700)new(112-118)new(280-294)recycled_nonces_zset_name(213-224)last_transaction_count_key_name(255-263)executors/src/eoa/store/borrowed.rs (4)
name(58-60)watch_keys(62-64)validation(66-92)operation(94-237)executors/src/eoa/store/pending.rs (8)
name(36-38)name(167-169)watch_keys(40-45)watch_keys(171-176)validation(47-119)validation(178-233)operation(121-145)operation(235-258)
executors/src/eoa/events.rs (5)
twmq/src/lib.rs (1)
job(157-162)executors/src/eoa/store/atomic.rs (1)
eoa(82-84)core/src/signer.rs (1)
nonce(365-365)executors/src/eoa/store/mod.rs (1)
nonce(368-373)executors/src/eoa/store/submitted.rs (1)
nonce(66-71)
executors/src/eoa/store/atomic.rs (7)
twmq/src/lib.rs (9)
redis(1184-1186)redis(1192-1192)redis(1284-1286)redis(1292-1292)redis(1322-1324)redis(1330-1332)name(174-176)pipeline(884-884)serde_json(776-776)executors/src/webhook/mod.rs (1)
queue_webhook_envelopes(433-505)executors/src/eoa/store/borrowed.rs (4)
name(58-60)operation(94-237)validation(66-92)watch_keys(62-64)executors/src/eoa/store/pending.rs (8)
name(36-38)name(167-169)operation(121-145)operation(235-258)validation(47-119)validation(178-233)watch_keys(40-45)watch_keys(171-176)executors/src/eoa/store/submitted.rs (9)
name(192-194)name(428-430)operation(218-351)operation(481-493)validation(200-216)validation(440-479)watch_keys(196-198)watch_keys(432-438)nonce(66-71)executors/src/eoa/store/mod.rs (7)
pipeline(700-700)from(338-345)from(349-354)from(787-791)from(795-800)serde_json(646-646)nonce(368-373)executors/src/eoa/worker/error.rs (2)
from(75-79)from(83-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (38)
server/src/http/routes/mod.rs (1)
1-1: LGTM! Clean module addition.The addition of the admin module declaration properly integrates the new administrative HTTP routes into the server's routing structure.
server/src/http/routes/sign_typed_data.rs (1)
146-146: LGTM! Efficient credential handling.Passing the signing credential by reference instead of cloning reduces unnecessary memory allocation and is consistent with the codebase-wide refactoring for improved efficiency.
server/src/http/routes/sign_message.rs (1)
123-123: LGTM! Consistent credential efficiency improvement.Passing the signing credential by reference maintains consistency with the codebase-wide optimization to reduce unnecessary cloning operations.
server/src/http/server.rs (1)
67-69: LGTM! Proper admin route integration.The new admin route for emptying queue idempotency sets is correctly integrated into the v1_router following the established routing patterns. The handler supports the expected queue types including the new eoa_executor queue.
aa-core/src/signer.rs (1)
164-164: LGTM! Comprehensive credential efficiency improvements.All four credential parameter changes consistently replace cloning with reference passing across the signing methods. This reduces unnecessary memory allocations in:
sign_message_with_factory_patternsign_typed_data_with_factory_pattern(both self-verifying and direct EOA paths)sign_712_wrapped_hashThe changes maintain functional equivalence while improving performance throughout the SmartAccountSigner implementation.
Also applies to: 192-192, 215-215, 245-245
server/src/http/routes/contract_write.rs (1)
59-61: LGTM! Good simplification of webhook options handling.The change from
Option<Vec<WebhookOptions>>toVec<WebhookOptions>with#[serde(default)]is well-designed. This eliminates the need to handleNonecases throughout the webhook processing pipeline while maintaining backward compatibility through the default attribute.core/src/execution_options/mod.rs (1)
92-94: Excellent consistency with the webhook options standardization.This change aligns
SendTransactionRequestwith the broader refactor to standardize webhook option handling. Using#[serde(default)]ensures that missing webhook options in incoming requests default to an empty vector, simplifying downstream processing.executors/src/eip7702_executor/confirm.rs (2)
35-36: Good alignment with the webhook options standardization pattern.The
#[serde(default)]attribute ensures proper deserialization behavior when webhook options are absent from the job data.
40-42: Trait implementation correctly updated to match the new signature.The method now returns
Vec<WebhookOptions>directly as expected by theHasWebhookOptionstrait, which aligns with the trait definition inexecutors/src/webhook/envelope.rsline 107.executors/src/external_bundler/confirm.rs (2)
37-37: Consistent implementation of the webhook options standardization.The change removes the
Optionwrapper and applies the default attribute, following the established pattern across executor modules.
341-343: Trait implementation properly aligned with the updated interface.The
webhook_optionsmethod correctly returns aVec<WebhookOptions>as expected by the trait definition, consistent with other executor implementations.executors/src/eip7702_executor/send.rs (4)
49-50: Consistent with the webhook options standardization across executors.The
#[serde(default)]attribute ensures proper handling of missing webhook options during job deserialization.
56-58: Trait implementation correctly updated for the new webhook options pattern.The method signature aligns with the
HasWebhookOptionstrait definition and follows the same implementation pattern as other executor modules.
247-247: Good performance optimization by avoiding unnecessary cloning.Passing
&job_data.signing_credentialby reference instead of cloning reduces memory allocation overhead in the signing operations.
276-276: Consistent signing credential optimization.Using a reference instead of cloning the signing credential improves performance and aligns with the change on line 247.
executors/src/external_bundler/send.rs (2)
52-53: Good refactoring to simplify webhook options handling.The change from
Option<Vec<WebhookOptions>>toVec<WebhookOptions>with#[serde(default)]is a solid improvement that:
- Eliminates unnecessary
Optionwrapping for collections- Maintains backward compatibility through default deserialization
- Aligns with the updated
HasWebhookOptionstrait signatureAlso applies to: 63-65
421-426: Improved code formatting for better readability.The reformatted iterator chain for calculating gas limits is clearer and easier to follow.
server/src/http/routes/admin/queue.rs (1)
44-134: Well-structured admin endpoint with proper error handling.The implementation includes:
- Appropriate logging for debugging and monitoring
- Clear error messages with context preservation
- Proper HTTP status codes
executors/src/eoa/store/pending.rs (2)
47-119: Excellent validation logic for atomic nonce operations.The validation method implements robust checks:
- Sequential nonce validation prevents gaps that could break transaction ordering
- Batch ZSCORE operations improve performance over individual checks
- Pre-serialization catches errors before the atomic operation
- Clear error messages aid debugging
121-146: Well-structured atomic operation maintaining consistency.The operation correctly updates all three Redis keys in a single pipeline, ensuring atomicity. The optimistic transaction count update (highest nonce + 1) maintains the invariant for future operations.
thirdweb-core/src/iaw/mod.rs (1)
217-219: Good optimization to reduce unnecessary cloning.The parameter changes from owned values to references improve efficiency without affecting functionality. The implementation correctly handles the references in format strings and method calls.
Also applies to: 299-301, 368-370, 438-441
executors/src/eoa/worker/send.rs (2)
217-264: Well-designed error handling with appropriate retry logic.The method effectively:
- Distinguishes between retryable and non-retryable errors
- Tracks when balance threshold updates are needed
- Immediately fails non-retryable transactions to prevent endless retries
1-402: Robust implementation of EOA transaction sending with excellent concurrency handling.The implementation demonstrates:
- Effective use of parallel processing for transaction building and sending
- Comprehensive error handling with appropriate retry strategies
- Good separation of concerns between recycled and new nonce processing
- Detailed tracing for operational visibility
The integration with atomic store operations aligns well with the WATCH-based coordination pattern mentioned in the retrieved learnings.
executors/src/eoa/store/hydrate.rs (1)
107-156: Well-structured implementation for hydrating submitted transactions.The method correctly handles special cases (no-op transactions) and validates that all required transactions exist in Redis before hydration.
core/src/signer.rs (1)
222-399: Implementation correctly updated for reference parameters.The implementations properly handle the new reference parameters, with necessary cloning only where required (e.g., vault client expects owned transaction).
executors/src/eoa/worker/transaction.rs (3)
39-89: Well-implemented retry logic with exponential backoff.The method properly handles retryable preparation errors with appropriate delays and logging.
113-142: Correct implementation of no-op transaction.The no-op transaction is properly constructed with minimal gas (21000) and zero value to efficiently consume recycled nonces.
237-350: Comprehensive transaction building with proper error handling.The method correctly handles different transaction types, estimates gas with a reasonable buffer (10%), and properly detects simulation failures.
executors/src/eoa/events.rs (1)
17-58: Well-structured event definitions.The event structures and enums provide clear types for different executor stages and error scenarios.
executors/src/eoa/worker/error.rs (3)
19-95: Comprehensive error handling framework.The error enum provides detailed error variants with proper context and implements necessary traits for serialization and error conversion.
111-156: Robust error classification with helpful unknown error logging.The classification logic comprehensively handles known error patterns and logs unknown errors for future improvement.
186-224: Well-designed retryability logic.The functions correctly distinguish between transient errors (retryable) and deterministic failures (non-retryable), following standard patterns for error handling.
executors/src/eoa/worker/confirm.rs (1)
27-159: Well-implemented confirmation flow with proper error handling.The confirmation flow correctly handles nonce progress tracking, stalled nonce detection with gas bumping, and transaction cleanup. The defensive approach of falling back to debug format when receipt serialization fails is a good practice.
executors/src/eoa/store/atomic.rs (1)
172-378: Excellent implementation of atomic Redis operations.The implementation correctly uses WATCH/MULTI/EXEC patterns with proper retry logic and lock validation. This aligns perfectly with the learnings about EOA executor store's comprehensive WATCH-based coordination, making aggressive lock acquisition safe.
executors/src/eoa/store/submitted.rs (1)
354-422: Excellent violation detection implementation.The
detect_violationsfunction comprehensively detects and categorizes various transaction anomalies including cross-nonce violations, multiple confirmations per nonce, and missing receipts. This is crucial for identifying potential issues like blockchain reorganizations or system bugs.executors/src/eoa/store/mod.rs (3)
1-28: LGTM! Well-structured imports and module organization.The imports are appropriate for a Redis-backed Ethereum transaction store, and the module structure follows good separation of concerns.
737-801: LGTM! Comprehensive and well-structured error handling.The error enum is well-designed with:
- Clear error variants for different failure modes
- Good use of thiserror for automatic trait implementations
- Proper conversions from underlying error types
- Structured error messages with relevant context
This provides excellent debugging information and follows Rust error handling best practices.
685-685: Apply defensive programming for timestamp casting.The current timestamp casting could overflow if
timestamp_millis()returns a negative value, which would wrap around when cast tou64.- let now = chrono::Utc::now().timestamp_millis().max(0) as u64; + let now = chrono::Utc::now().timestamp_millis().max(0) as u64;Actually, let me provide a safer approach:
- let now = chrono::Utc::now().timestamp_millis().max(0) as u64; + let now = match chrono::Utc::now().timestamp_millis() { + ts if ts >= 0 => ts as u64, + _ => 0, // Default to epoch for negative timestamps + };⛔ Skipped due to learnings
Learnt from: d4mr PR: thirdweb-dev/engine-core#14 File: executors/src/webhook/mod.rs:439-439 Timestamp: 2025-07-12T01:46:22.885Z Learning: The user prefers defensive programming approaches to prevent overflow issues when casting between integer types, specifically when converting i64 timestamps to u64 to avoid wraparound from negative values.
| auth_token: &AuthToken, | ||
| thirdweb_auth: &ThirdwebAuth, | ||
| message: &str, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify inconsistent parameter handling in sign_userop.
While other methods have been updated to accept references for auth_token and thirdweb_auth, the sign_userop method (lines 512-517) still takes owned values. This inconsistency should be addressed for API consistency.
Also applies to: 299-301, 368-370, 438-441, 512-517
🏁 Script executed:
#!/bin/bash
# Description: Check if sign_userop is called anywhere and if ownership is required
# Search for calls to sign_userop to understand usage patterns
rg -A 5 "sign_userop\(" --type rust
# Check if auth_token or thirdweb_auth are moved/consumed within sign_userop
ast-grep --pattern 'pub async fn sign_userop($$$) {
$$$
}'Length of output: 820
🏁 Script executed:
#!/bin/bash
# Extract the sign_userop function signature and first part of its body to inspect usage of auth_token and thirdweb_auth
rg -n 'pub async fn sign_userop' -A 60 thirdweb-core/src/iaw/mod.rsLength of output: 2592
Align sign_userop with other methods by taking references
The other IAW methods now accept &AuthToken and &ThirdwebAuth for efficiency and consistency. sign_userop should do the same.
• In thirdweb-core/src/iaw/mod.rs (lines 512–513):
- Change
to
auth_token: AuthToken, thirdweb_auth: ThirdwebAuth,
auth_token: &AuthToken, thirdweb_auth: &ThirdwebAuth,
• Update call sites (e.g. in core/src/userop.rs):
- Replace
with
.sign_userop( auth_token.clone(), thirdweb_auth.clone(), /* … */ )
.sign_userop( &auth_token, &thirdweb_auth, /* … */ )
These changes remove unnecessary clones and keep the API surface consistent.
🤖 Prompt for AI Agents
In thirdweb-core/src/iaw/mod.rs around lines 512 to 513, change the parameters
of the sign_userop function from owning AuthToken and ThirdwebAuth to taking
references (&AuthToken and &ThirdwebAuth) for consistency with other methods.
Then, update all call sites such as in core/src/userop.rs to pass references
(&auth_token, &thirdweb_auth) instead of cloning the objects. This removes
unnecessary cloning and aligns the API for efficiency.
| // TODO: refactor this, very ugly | ||
| if health.balance <= health.balance_threshold { | ||
| if now - health.balance_fetched_at > HEALTH_CHECK_INTERVAL { | ||
| let balance = self | ||
| .chain | ||
| .provider() | ||
| .get_balance(self.eoa) | ||
| .await | ||
| .map_err(|e| { | ||
| let engine_error = e.to_engine_error(&self.chain); | ||
| EoaExecutorWorkerError::RpcError { | ||
| message: format!("Failed to get balance: {}", engine_error), | ||
| inner_error: engine_error, | ||
| } | ||
| })?; | ||
|
|
||
| health.balance = balance; | ||
| health.balance_fetched_at = now; | ||
| self.store.update_health_data(&health).await?; | ||
| } | ||
|
|
||
| if health.balance <= health.balance_threshold { | ||
| tracing::warn!( | ||
| "EOA has insufficient balance (<= {} wei), skipping send flow", | ||
| health.balance_threshold | ||
| ); | ||
| return Ok(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the balance check logic as indicated by TODO.
The nested balance check logic is complex and could be simplified for better readability and maintainability.
-// TODO: refactor this, very ugly
-if health.balance <= health.balance_threshold {
- if now - health.balance_fetched_at > HEALTH_CHECK_INTERVAL {
- let balance = self
- .chain
- .provider()
- .get_balance(self.eoa)
- .await
- .map_err(|e| {
- let engine_error = e.to_engine_error(&self.chain);
- EoaExecutorWorkerError::RpcError {
- message: format!("Failed to get balance: {}", engine_error),
- inner_error: engine_error,
- }
- })?;
-
- health.balance = balance;
- health.balance_fetched_at = now;
- self.store.update_health_data(&health).await?;
- }
-
- if health.balance <= health.balance_threshold {
- tracing::warn!(
- "EOA has insufficient balance (<= {} wei), skipping send flow",
- health.balance_threshold
- );
- return Ok(0);
- }
-}
+// Update balance if stale
+let should_update_balance = now.saturating_sub(health.balance_fetched_at) > HEALTH_CHECK_INTERVAL;
+if should_update_balance || health.balance <= health.balance_threshold {
+ let balance = self
+ .chain
+ .provider()
+ .get_balance(self.eoa)
+ .await
+ .map_err(|e| {
+ let engine_error = e.to_engine_error(&self.chain);
+ EoaExecutorWorkerError::RpcError {
+ message: format!("Failed to get balance: {}", engine_error),
+ inner_error: engine_error,
+ }
+ })?;
+
+ health.balance = balance;
+ health.balance_fetched_at = now;
+ self.store.update_health_data(&health).await?;
+}
+
+// Check if balance is sufficient
+if health.balance <= health.balance_threshold {
+ tracing::warn!(
+ "EOA has insufficient balance ({} <= {} wei), skipping send flow",
+ health.balance,
+ health.balance_threshold
+ );
+ return Ok(0);
+}🤖 Prompt for AI Agents
In executors/src/eoa/worker/send.rs between lines 26 and 54, the nested if
statements checking the balance and timing conditions make the code hard to
read. Refactor by combining the conditions where possible and reducing nesting,
for example by returning early when conditions fail, and consolidating the
balance threshold checks after updating the balance. This will simplify the flow
and improve readability and maintainability.
| let results: Vec<String> = pipe.query_async(&mut self.redis.clone()).await?; | ||
|
|
||
| let mut hydrated = Vec::with_capacity(dehydrated.len()); | ||
| for (d, r) in dehydrated.into_iter().zip(results.iter()) { | ||
| hydrated.push(d.hydrate(serde_json::from_str::<EoaTransactionRequest>(r)?)); | ||
| } |
There was a problem hiding this comment.
Add validation for Redis query results length.
The method assumes Redis returns exactly one result per query, but if some keys are missing, the zip operation will silently drop excess dehydrated items. This could lead to data loss.
Add this validation after line 83:
let results: Vec<String> = pipe.query_async(&mut self.redis.clone()).await?;
+
+if results.len() != dehydrated.len() {
+ return Err(TransactionStoreError::InternalError {
+ message: format!(
+ "Redis returned {} results but expected {}",
+ results.len(),
+ dehydrated.len()
+ ),
+ });
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let results: Vec<String> = pipe.query_async(&mut self.redis.clone()).await?; | |
| let mut hydrated = Vec::with_capacity(dehydrated.len()); | |
| for (d, r) in dehydrated.into_iter().zip(results.iter()) { | |
| hydrated.push(d.hydrate(serde_json::from_str::<EoaTransactionRequest>(r)?)); | |
| } | |
| let results: Vec<String> = pipe.query_async(&mut self.redis.clone()).await?; | |
| if results.len() != dehydrated.len() { | |
| return Err(TransactionStoreError::InternalError { | |
| message: format!( | |
| "Redis returned {} results but expected {}", | |
| results.len(), | |
| dehydrated.len() | |
| ), | |
| }); | |
| } | |
| let mut hydrated = Vec::with_capacity(dehydrated.len()); | |
| for (d, r) in dehydrated.into_iter().zip(results.iter()) { | |
| hydrated.push(d.hydrate(serde_json::from_str::<EoaTransactionRequest>(r)?)); | |
| } |
🤖 Prompt for AI Agents
In executors/src/eoa/store/hydrate.rs around lines 83 to 88, add a validation
step after retrieving the Redis query results to check that the length of the
results vector matches the length of the dehydrated vector. If the lengths
differ, return an error or handle the mismatch appropriately to prevent silent
data loss caused by the zip operation dropping unmatched items. This ensures
that every dehydrated item has a corresponding Redis result before proceeding
with hydration.
| pub fn apply_gas_bump_to_typed_transaction( | ||
| &self, | ||
| mut typed_tx: TypedTransaction, | ||
| bump_multiplier: u32, // e.g., 120 for 20% increase | ||
| ) -> TypedTransaction { | ||
| match &mut typed_tx { | ||
| TypedTransaction::Eip1559(tx) => { | ||
| tx.max_fee_per_gas = tx.max_fee_per_gas * bump_multiplier as u128 / 100; | ||
| tx.max_priority_fee_per_gas = | ||
| tx.max_priority_fee_per_gas * bump_multiplier as u128 / 100; | ||
| } | ||
| TypedTransaction::Legacy(tx) => { | ||
| tx.gas_price = tx.gas_price * bump_multiplier as u128 / 100; | ||
| } | ||
| TypedTransaction::Eip2930(tx) => { | ||
| tx.gas_price = tx.gas_price * bump_multiplier as u128 / 100; | ||
| } | ||
| TypedTransaction::Eip7702(tx) => { | ||
| tx.max_fee_per_gas = tx.max_fee_per_gas * bump_multiplier as u128 / 100; | ||
| tx.max_priority_fee_per_gas = | ||
| tx.max_priority_fee_per_gas * bump_multiplier as u128 / 100; | ||
| } | ||
| TypedTransaction::Eip4844(tx) => match tx { | ||
| TxEip4844Variant::TxEip4844(tx) => { | ||
| tx.max_fee_per_gas = tx.max_fee_per_gas * bump_multiplier as u128 / 100; | ||
| tx.max_priority_fee_per_gas = | ||
| tx.max_priority_fee_per_gas * bump_multiplier as u128 / 100; | ||
| } | ||
| TxEip4844Variant::TxEip4844WithSidecar(TxEip4844WithSidecar { tx, .. }) => { | ||
| tx.max_fee_per_gas = tx.max_fee_per_gas * bump_multiplier as u128 / 100; | ||
| tx.max_priority_fee_per_gas = | ||
| tx.max_priority_fee_per_gas * bump_multiplier as u128 / 100; | ||
| } | ||
| }, | ||
| } | ||
| typed_tx | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for bump_multiplier parameter.
The method should validate that bump_multiplier is reasonable to prevent accidental gas price reduction.
Add validation at the beginning of the method:
pub fn apply_gas_bump_to_typed_transaction(
&self,
mut typed_tx: TypedTransaction,
bump_multiplier: u32, // e.g., 120 for 20% increase
) -> TypedTransaction {
+ // Ensure multiplier is at least 100 to avoid reducing gas
+ let bump_multiplier = bump_multiplier.max(100);
+
match &mut typed_tx {🤖 Prompt for AI Agents
In executors/src/eoa/worker/transaction.rs around lines 390 to 426, the
apply_gas_bump_to_typed_transaction method lacks validation for the
bump_multiplier parameter, which could lead to unintended gas price reductions.
Add a validation check at the start of the method to ensure bump_multiplier is
greater than or equal to 100 (representing no decrease or an increase) and
handle invalid values appropriately, such as returning an error or panicking
with a clear message.
| pub fn transaction_data_key_name(&self, transaction_id: &str) -> String { | ||
| match &self.namespace { | ||
| Some(ns) => format!("{ns}:eoa_executor:tx_data:{transaction_id}"), | ||
| None => format!("eoa_executor:_tx_data:{transaction_id}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix key naming inconsistency.
There's an inconsistency in the transaction data key naming between namespaced and non-namespaced cases.
- None => format!("eoa_executor:_tx_data:{transaction_id}"),
+ None => format!("eoa_executor:tx_data:{transaction_id}"),🤖 Prompt for AI Agents
In executors/src/eoa/store/mod.rs around lines 137 to 142, the
transaction_data_key_name function has inconsistent key naming between the
Some(namespace) and None cases. To fix this, ensure the key format is consistent
by removing the underscore in the None case so both keys follow the pattern
"{namespace}:eoa_executor:tx_data:{transaction_id}" or
"eoa_executor:tx_data:{transaction_id}" without extra underscores.
| if let Ok(attempt) = serde_json::from_str::<TransactionAttempt>(&attempt_json) { | ||
| attempts.push(attempt); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for corrupted transaction attempts.
Individual attempt deserialization failures are silently ignored, which could mask data corruption issues.
let mut attempts = Vec::new();
for attempt_json in attempts_json_list {
- if let Ok(attempt) = serde_json::from_str::<TransactionAttempt>(&attempt_json) {
- attempts.push(attempt);
- }
+ match serde_json::from_str::<TransactionAttempt>(&attempt_json) {
+ Ok(attempt) => attempts.push(attempt),
+ Err(e) => {
+ tracing::warn!(
+ transaction_id = %transaction_id,
+ error = %e,
+ "Failed to deserialize transaction attempt, skipping"
+ );
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Ok(attempt) = serde_json::from_str::<TransactionAttempt>(&attempt_json) { | |
| attempts.push(attempt); | |
| } | |
| } | |
| let mut attempts = Vec::new(); | |
| for attempt_json in attempts_json_list { | |
| match serde_json::from_str::<TransactionAttempt>(&attempt_json) { | |
| Ok(attempt) => attempts.push(attempt), | |
| Err(e) => { | |
| tracing::warn!( | |
| transaction_id = %transaction_id, | |
| error = %e, | |
| "Failed to deserialize transaction attempt, skipping" | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In executors/src/eoa/store/mod.rs around lines 646 to 649, the code currently
ignores errors when deserializing individual TransactionAttempt entries, which
can hide data corruption problems. Modify the code to handle deserialization
errors explicitly by logging a warning or error message when an attempt fails to
deserialize, instead of silently ignoring it. This will help surface corrupted
data issues during processing.
| for (_transaction_id, transaction_json) in borrowed_map { | ||
| let borrowed_data: BorrowedTransactionData = serde_json::from_str(&transaction_json)?; | ||
| result.push(borrowed_data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle individual transaction deserialization failures gracefully.
If one borrowed transaction is corrupted, the entire operation fails. Consider handling individual failures more gracefully.
for (_transaction_id, transaction_json) in borrowed_map {
- let borrowed_data: BorrowedTransactionData = serde_json::from_str(&transaction_json)?;
- result.push(borrowed_data);
+ match serde_json::from_str::<BorrowedTransactionData>(&transaction_json) {
+ Ok(borrowed_data) => result.push(borrowed_data),
+ Err(e) => {
+ tracing::warn!(
+ error = %e,
+ "Failed to deserialize borrowed transaction, skipping"
+ );
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (_transaction_id, transaction_json) in borrowed_map { | |
| let borrowed_data: BorrowedTransactionData = serde_json::from_str(&transaction_json)?; | |
| result.push(borrowed_data); | |
| } | |
| for (_transaction_id, transaction_json) in borrowed_map { | |
| match serde_json::from_str::<BorrowedTransactionData>(&transaction_json) { | |
| Ok(borrowed_data) => result.push(borrowed_data), | |
| Err(e) => { | |
| tracing::warn!( | |
| error = %e, | |
| "Failed to deserialize borrowed transaction, skipping" | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In executors/src/eoa/store/mod.rs around lines 430 to 433, the current code
deserializes each transaction JSON string directly and returns an error if any
single deserialization fails, causing the entire operation to fail. Modify the
loop to handle deserialization errors for each transaction individually by using
a match or if let to attempt deserialization, pushing successfully deserialized
items to the result vector, and logging or skipping corrupted entries without
stopping the entire process.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
executors/src/eoa/worker/mod.rs (1)
336-339: Fix incorrect field mappings in result construction.Both
failed_transactionsandreplaced_transactionsare incorrectly assigned the same value frommoved_to_pending. Themoved_to_pendingfield semantically represents replaced transactions, not failed ones.- failed_transactions: confirmations_report.moved_to_pending as u32, + failed_transactions: confirmations_report.failed_transactions as u32, sent_transactions: sent, replaced_transactions: confirmations_report.moved_to_pending as u32,Note: Ensure that
confirmations_reportincludes afailed_transactionsfield or adjust the logic accordingly.executors/src/eoa/store/atomic.rs (1)
275-276: Fix incorrect example in documentation.The example shows passing
worker_idas a parameter, but the method signature doesn't include this parameter.-/// self.execute_with_watch_and_retry(, worker_id, &safe_tx).await?; +/// self.execute_with_watch_and_retry(&safe_tx).await?;
🧹 Nitpick comments (2)
executors/src/eoa/worker/mod.rs (2)
401-408: Address TODO comments before production deployment.The TODO comments indicate missing post-processing analysis that was previously handled in individual atomic operations. This could impact error handling and nonce management in production.
Consider implementing the missing functionality:
- Analyzing submission results for specific error patterns
- Calling
update_balance_thresholdwhen needed- Detecting nonce reset conditions
Do you want me to help implement this post-processing logic or create an issue to track this work?
181-188: Remove commented-out code before production.The commented-out code should be removed to maintain code cleanliness, especially for production deployment.
- // // initiate health data if doesn't exist - // self.get_eoa_health(&scoped, &chain) - // .await - // .map_err_nack(Some(Duration::from_secs(10)), RequeuePosition::Last)?; - - // // Execute main workflow with proper error handling - // self.execute_main_workflow(&scoped, &chain).await
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
executors/src/eoa/store/atomic.rs(1 hunks)executors/src/eoa/store/submitted.rs(1 hunks)executors/src/eoa/worker/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- executors/src/eoa/store/submitted.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
executors/src/eoa/store/atomic.rs (3)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#14
File: executors/src/eoa/store/atomic.rs:38-42
Timestamp: 2025-07-13T20:57:25.741Z
Learning: As of Rust 1.75 (released December 2023), you can use `impl Trait` as the return type of trait methods, including `impl Future` for async-like behavior without requiring the async-trait crate. This eliminates the need for async-trait in many cases where traits need async-like methods.
executors/src/eoa/worker/mod.rs (2)
Learnt from: d4mr
PR: thirdweb-dev/engine-core#5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: In the EOA executor system, aggressive lock acquisition is safe because every Redis state mutation uses WATCH operations on the lock key. If the lock is lost during a transaction, the WATCH causes the transaction to fail and the worker exits gracefully. This provides coordination between workers even when using forceful lock takeover.
Learnt from: d4mr
PR: thirdweb-dev/engine-core#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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (5)
executors/src/eoa/worker/mod.rs (1)
154-156: Verify aggressive lock acquisition behavior.The aggressive lock acquisition aligns with the retrieved learnings about WATCH-based coordination. The implementation correctly relies on the atomic store's WATCH operations to handle lock validation during state mutations.
This approach is safe because every Redis state mutation will validate lock ownership through WATCH operations, causing transactions to fail if the lock is lost.
executors/src/eoa/store/atomic.rs (4)
33-49: LGTM on modern Rust trait design.The
SafeRedisTransactiontrait correctly usesimpl Futurereturn types, which is supported in Rust 1.75+. This avoids the need for the async-trait crate and provides clean separation between async validation and sync pipeline operations.
322-339: Excellent WATCH-based coordination implementation.The lock checking and WATCH operations align perfectly with the retrieved learnings about comprehensive WATCH-based coordination. The implementation correctly:
- Watches the lock key along with operation-specific keys
- Validates lock ownership before proceeding
- Fails fast with
LockLosterror if ownership is lost- Uses proper UNWATCH cleanup on validation failures
This design ensures that only the actual lock holder can perform state mutations, making aggressive lock acquisition safe as learned from previous implementations.
295-306: Robust retry logic with proper error context.The exponential backoff and retry logic provides good resilience while including helpful context in error messages (operation name, EOA, chain ID).
97-137: Proper lock release implementation following spec pattern.The
release_eoa_lockmethod correctly implements the spec's "finally pattern" with graceful handling of lock loss scenarios. The error handling appropriately logs warnings without failing the worker when lock release encounters issues.
Summary by CodeRabbit
New Features
Improvements