feat: read_transaction in InMemoryTemporaryStorage

[dinhani-cw]

No description provided.

[github-actions]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	3
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Logging and Monitoring: Ensure that the new methods push_transaction and read_transaction in PendingBlock and InMemoryTemporaryStorage are properly logged and monitored, especially since these methods are critical for transaction management.
	Error Handling: The method read_transaction in InMemoryTemporaryStorage uses pattern matching with Some(ref pending_block). Ensure that there is proper error handling or logging when None is encountered.
	Performance Concern: The change from Vec to IndexMap for tx_executions in PendingBlock might have implications on performance due to different data structure characteristics. Review the performance especially if the transaction volume is high.

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add a check to prevent overwriting existing transactions in the block Consider checking if the Hash key already exists in tx_executions before inserting a new transaction. This can prevent overwriting existing entries unintentionally. src/eth/primitives/pending_block.rs [27] +if self.tx_executions.contains_key(&tx.hash()) { + return Err(anyhow::Error::new(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "Transaction already exists."))); +} self.tx_executions.insert(tx.hash(), tx); Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential issue where existing transactions could be unintentionally overwritten, which is a significant concern. The proposed change adds a check to prevent this, improving the robustness of the code.	9
Possible bug	Handle the case where there is no active block to push transactions to Add error handling for the case where require_active_block_mut() returns None, to avoid potential panics when there is no active block. src/eth/storage/inmemory/inmemory_temporary.rs [188] -states.head.require_active_block_mut()?.push_transaction(tx); +let active_block = states.head.require_active_block_mut()?; +if active_block.is_none() { + return Err(anyhow::Error::new(std::io::Error::new(std::io::ErrorKind::NotFound, "No active block available."))); +} +active_block.unwrap().push_transaction(tx); Suggestion importance[1-10]: 8 Why: This suggestion addresses a possible bug where the absence of an active block could lead to a panic. Adding error handling for this case improves the stability and reliability of the code.	8
	Add error handling for missing pending block to prevent potential panics Implement error handling for the case when pending_block is None to avoid unwrapping a None value which can lead to panic. src/eth/storage/inmemory/inmemory_temporary.rs [212-215] -let Some(ref pending_block) = states.head.block else { return Ok(None) }; -match pending_block.tx_executions.get(hash) { - Some(tx) => Ok(Some(tx.clone())), - None => Ok(None), +if let Some(ref pending_block) = states.head.block { + match pending_block.tx_executions.get(hash) { + Some(tx) => Ok(Some(tx.clone())), + None => Ok(None), + } +} else { + Err(anyhow::Error::new(std::io::Error::new(std::io::ErrorKind::NotFound, "No pending block available."))) } Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential bug where unwrapping a None value could lead to a panic. Adding error handling for this case improves the robustness and reliability of the code.	8
Performance	Use vector extend method for more efficient element addition Consider using extend instead of push in loops to add elements to vectors, which can be more efficient when adding multiple elements. src/eth/primitives/pending_block.rs [35-38] -for tx in self.tx_execotions.values().cloned() { - match tx { - TransactionExecution::Local(tx) => { - local_txs.push(tx); - } - TransactionExecution::External(tx) => { - external_txs.push(tx); - } - } -} +local_txs.extend(self.tx_executions.values().cloned().filter_map(|tx| if let TransactionExecution::Local(ltx) = tx { Some(ltx) } else { None })); +external_txs.extend(self.tx_executions.values().cloned().filter_map(|tx| if let TransactionExecution::External(etx) = tx { Some(etx) } else { None })); Suggestion importance[1-10]: 6 Why: While this suggestion can improve performance by using extend instead of push, the performance gain might be marginal in this context. It is a good optimization but not critical.	6