refactor: simplify temporary storage

[dinhani-cw]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR involves multiple changes across several files, including refactoring and feature removals which require careful consideration of impacts on existing functionality and dependencies. The changes are spread across core components like mining logic and storage handling, which are critical and complex parts of the system.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The removal of RocksTemporaryStorage without a clear replacement or migration path could lead to issues if other parts of the system still depend on this functionality.
	Possible Bug: The new method mine_external_mixed in BlockMiner assumes that the block exists (let Some(external_block) = block.external_block else { ... }), which might not always be the case, potentially leading to runtime errors if not handled properly.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/block_miner.rs
suggestion	Consider adding error handling for the case when block.external_block is None in the mine_external method to prevent potential runtime panics. [important]
relevant line	let Some(external_block) = block.external_block else {

---

relevant file	src/eth/block_miner.rs
suggestion	Refactor the repeated logic in mine_external and mine_external_mixed methods to a common function to improve code maintainability and reduce duplication. [important]
relevant line	let mined_txs = mine_external_transactions(block.number, external_txs)?;

---

relevant file	src/eth/storage/inmemory/inmemory_temporary.rs
suggestion	Ensure that the new block number is correctly incremented in finish_block method to maintain the integrity of block sequencing. [important]
relevant line	states.head.block = Some(PendingBlock::new(finished_block.number.next()));

---

relevant file	src/bin/importer_online.rs
suggestion	Verify that the new method mine_external_mixed is correctly handling all edge cases, especially around transaction types and block commitments, to ensure system stability. [important]
relevant line	let mined_block = miner.mine_external_mixed().await?;

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Maintainability	Refactor require_active_block and require_active_block_mut to reduce code redundancy The require_active_block_mut and require_active_block methods duplicate some logic. Consider refactoring these methods to reduce redundancy and improve maintainability. src/eth/storage/inmemory/inmemory_temporary.rs [77-89] -fn require_active_block(&mut self) -> anyhow::Result<&PendingBlock> { +fn require_active_block<T>(&mut self) -> anyhow::Result<&T> where T: Borrow<PendingBlock> + BorrowMut<PendingBlock> { match &self.block { - Some(block) => Ok(block), - None => log_and_err!("no pending block being mined"), - } -} -fn require_active_block_mut(&mut self) -> anyhow::Result<&mut PendingBlock> { - match &mut self.block { - Some(block) => Ok(block), + Some(block) => Ok(block.borrow()), None => log_and_err!("no pending block being mined"), } } Suggestion importance[1-10]: 8 Why: The suggestion to refactor require_active_block and require_active_block_mut is beneficial for maintainability by reducing code redundancy and simplifying future modifications.	8
Possible issue	Add error handling for the mine_external_transactions function to enhance robustness Consider handling the case where mine_external_transactions might fail due to network or other errors. Currently, the function assumes success and proceeds to create a block from external transactions. Adding error handling here would make the function more robust. src/eth/block_miner.rs [59] -let mined_txs = mine_external_transactions(block.number, external_txs)?; +let mined_txs = mine_external_transactions(block.number, external_txs) + .map_err(|e| log_and_err!("failed to mine external transactions: {}", e))?; Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies a potential issue with error handling in the mine_external_transactions function, which is crucial for robustness, especially in network-related operations.	7
Enhancement	Implement filtering of local transactions to include only failed ones in mine_external_mixed The current implementation of mine_external_mixed does not differentiate between successful and failed local transactions as mentioned in the comment. It's important to implement this differentiation to ensure that only failed local transactions are included. src/eth/block_miner.rs [76-77] let mined_txs = mine_external_transactions(block.number, external_txs)?; -let mut block = block_from_external(external_block, mined_txs)?; +let failed_local_txs = filter_failed_local_transactions(local_txs); +let mut block = block_from_external_and_local(external_block, mined_txs, failed_local_txs)?; Suggestion importance[1-10]: 6 Why: The suggestion is valid as it addresses the need to differentiate between successful and failed local transactions in the mine_external_mixed method, aligning with the method's documentation and intent.	6