feat: reexecute_external returns transaction and receipt instead of keeping track of transaction index

[dinhani-cw]

No description provided.

[github-actions]

PR Review

⏱️ Estimated effort to review [1-5]	4, due to the extensive changes across multiple files and the complexity of the modifications involving transaction execution logic, error handling, and struct transformations. The PR affects core functionalities and requires a thorough understanding of the existing architecture and the changes to ensure correctness and performance.
🧪 Relevant tests	No
🔍 Possible issues	Possible Bug: The new EvmExecutionResult struct is used extensively, but there is no clear indication if all edge cases, such as null values or incorrect types, are handled properly in the new execution flow.
	Performance Concern: The changes in parallel execution logic could introduce performance bottlenecks or race conditions that were not present in the original implementation.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/executor.rs
suggestion	Consider handling the potential unwrap panics when converting receipt.gas_used.unwrap_or_default().try_into(). Using unwrap_or_default() assumes that there will always be a value or a default, but try_into() can still fail if the default or value does not fit into the target type. It's safer to handle this potential error explicitly to avoid runtime panics. [important]
relevant line	evm_result.execution.gas = match receipt.gas_used.unwrap_or_default().try_into() {

---

relevant file	src/eth/executor.rs
suggestion	Refactor the error handling in reexecute_external to reduce code duplication. Each match arm that handles errors ends with similar return (tx, receipt, Err(e)) statements. This could be streamlined by capturing the error early and handling it once at the end of the function. [medium]
relevant line	Err(e) => return (tx, receipt, Err(e)),

---

relevant file	src/eth/executor.rs
suggestion	In the reexecute_external function, consider adding detailed logging for each step of the transaction re-execution process to aid in debugging and provide clearer insights into the flow and any potential issues. [medium]
relevant line	let evm_result = if receipt.is_success() {

---

relevant file	src/eth/executor.rs
suggestion	Optimize the handling of prev_execution in the parallel execution logic. Currently, the code clones the execution result for each transaction, which could be inefficient. Consider using references or other strategies to reduce the overhead. [medium]
relevant line	prev_execution = Some(evm_result.execution.clone());

---

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[github-actions]

PR Code Suggestions

Category	Suggestions
Enhancement	Improve error handling for missing gas_used in receipts. Consider handling the case where receipt.gas_used is None more explicitly before unwrapping with unwrap_or_default(), which might lead to incorrect gas values being used. src/eth/executor.rs [257-259] -evm_result.execution.gas = match receipt.gas_used.unwrap_or_default().try_into() { - Ok(gas) => gas, - Err(e) => return (tx, receipt, Err(e)), +evm_result.execution.gas = match receipt.gas_used { + Some(gas_used) => match gas_used.try_into() { + Ok(gas) => gas, + Err(e) => return (tx, receipt, Err(e)), + }, + None => { + tracing::warn!("Gas used is not available in the receipt."); + return (tx, receipt, Err(anyhow::Error::new(std::io::Error::new(std::io::ErrorKind::NotFound, "Gas used not found")))); + } };
	Improve error reporting for EVM input creation failures. Consider using a more specific error type or message when handling errors from EvmInput::from_external_transaction to provide clearer debugging information. src/eth/executor.rs [230-233] let evm_input = match EvmInput::from_external_transaction(tx, receipt, block) { Ok(evm_input) => evm_input, - Err(e) => return (tx, receipt, Err(e)), + Err(e) => { + tracing::error!("Failed to create EVM input from external transaction: {:?}", e); + return (tx, receipt, Err(e)); + }, };
Best practice	Handle potential serialization errors gracefully. Replace the direct unwrapping of JSON serialization with error handling to avoid potential panics in production code. src/eth/executor.rs [265-267] -let json_tx = serde_json::to_string(&tx).unwrap(); -let json_receipt = serde_json::to_string(&receipt).unwrap(); -let json_execution_logs = serde_json::to_string(&evm_result.execution.logs).unwrap(); +let json_tx = serde_json::to_string(&tx).map_err(|e| anyhow::Error::new(e))?; +let json_receipt = serde_json::to_string(&receipt).map_err(|e| anyhow::Error::new(e))?; +let json_execution_logs = serde_json::to_string(&evm_result.execution.logs).map_err(|e| anyhow::Error::new(e))?;
	Ensure thread safety in parallel execution handling. To avoid potential race conditions and ensure thread safety, consider using atomic operations or locks when modifying shared state such as prev_execution in the parallel execution loop. src/eth/executor.rs [198] -prev_execution = Some(evm_result.execution.clone()); +{ + let mut lock = prev_execution.lock().await; + *lock = Some(evm_result.execution.clone()); +}
Maintainability	Refactor error handling into a helper function for clarity and reusability. Refactor the repeated pattern of error handling in reexecute_external to a helper function to improve code maintainability and reduce duplication. src/eth/executor.rs [236-243] -let sender = match self.storage.read_account(&receipt.from.into(), &StoragePointInTime::Present).await { - Ok(sender) => sender, - Err(e) => return (tx, receipt, Err(e)), -}; -let execution = match Execution::from_failed_external_transaction(sender, receipt, block) { - Ok(execution) => execution, - Err(e) => return (tx, receipt, Err(e)), -}; +async fn handle_error<T>(result: Result<T, anyhow::Error>, tx: &'b ExternalTransaction, receipt: &'b ExternalReceipt) -> Result<T, (&'b ExternalTransaction, &'b ExternalReceipt, anyhow::Error)> { + match result { + Ok(value) => Ok(value), + Err(e) => Err((tx, receipt, e)), + } +} +let sender = handle_error(self.storage.read_account(&receipt.from.into(), &StoragePointInTime::Present).await, tx, receipt)?; +let execution = handle_error(Execution::from_failed_external_transaction(sender, receipt, block), tx, receipt)?;

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.