forward to without execution

[renancloudwalk]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR involves significant changes across multiple files and modules, including configuration, execution logic, and transaction relaying. The changes affect core functionalities such as transaction execution and forwarding, requiring a thorough understanding of the system's architecture and the implications of removing storage dependencies.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The removal of storage handling in TransactionRelayer and IntegratedRelayerConfig might lead to unhandled cases where transactions need to be retried or logged due to failures. This could affect the system's ability to recover from errors or maintain a consistent state.
	Design Concern: The removal of the relayer's presence check in Executor simplifies the code but assumes that transactions no longer need to be forwarded or handled differently if a relayer is configured. This could limit the flexibility or expected functionality of the system.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/executor.rs
suggestion	Consider implementing a mechanism to handle retries or a stop condition for the transaction execution loop to prevent potential infinite loops or system hang-ups. This is crucial especially since the TODO comment suggests adding a timeout or max retries which is not yet implemented. [important]
relevant line	// TODO: must have a stop condition like timeout or max number of retries

---

relevant file	src/eth/relayer/internal.rs
suggestion	Implement error handling for the new forwarding logic in TransactionRelayer. Currently, if the transaction forwarding fails, there is no retry or error logging mechanism, which could lead to lost transactions or unreported errors. [important]
relevant line	//TODO send the result of the tx back

---

relevant file	src/eth/rpc/rpc_server.rs
suggestion	Add logic to check the forward_to configuration before deciding whether to execute the transaction locally or just forward it. This ensures that the system behaves correctly according to the configuration and can help in reducing unnecessary transaction executions. [important]
relevant line	//TODO if the forward_to is up, dont call executor, just forward it

---

relevant file	src/config.rs
suggestion	Restore or refactor the handling of storage in IntegratedRelayerConfig if it is essential for other operations or state management within the system, or ensure that all functionalities that depended on storage are appropriately adjusted to work without it. [medium]
relevant line	pub async fn init(&self) -> anyhow::Result>>

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a stop condition to the loop to prevent potential infinite execution The loop in the Executor implementation lacks a stop condition, which could lead to infinite loops or resource exhaustion under certain conditions. Implement a timeout or maximum retry limit to prevent such issues. src/eth/executor.rs [273-291] +let mut retry_count = 0; +let max_retries = 10; // Example max retries loop { + if retry_count >= max_retries { + return Err(anyhow!("Max retries exceeded")); + } let evm_input = EvmInput::from_eth_transaction(tx_input.clone()); let evm_result = self.execute_in_evm(evm_input).await?; let tx_execution = TransactionExecution::new_local(tx_input.clone(), evm_result.clone()); if let Err(e) = self.miner.save_execution(tx_execution.clone()).await { if let Some(StorageError::Conflict(conflicts)) = e.downcast_ref::<StorageError>() { tracing::warn!(?conflicts, "temporary storage conflict detected when saving execution"); + retry_count += 1; continue; } else { return Err(e); } } break tx_execution; } Suggestion importance[1-10]: 9 Why: This suggestion addresses a critical issue of potential infinite loops by proposing a retry limit, which significantly improves the robustness of the code.	9
Enhancement	Implement error handling and status checks in the forward method to ensure transaction integrity The forward method in TransactionRelayer currently does not handle transaction failures or check the transaction status after forwarding. Implement error handling and status checks to ensure that transactions are successfully forwarded and recorded. src/eth/relayer/internal.rs [27-36] pub async fn forward(&self, tx_input: TransactionInput) -> anyhow::Result<()> { let tx = self.chain.send_raw_transaction(tx_input.hash, Transaction::from(tx_input.clone()).rlp()).await?; + let receipt = tx.await?; + if receipt.status != Some(1) { + return Err(anyhow!("Transaction failed on the external blockchain")); + } Ok(()) } Suggestion importance[1-10]: 8 Why: This suggestion enhances the reliability of the forward method by adding error handling and status checks, which are crucial for ensuring transaction integrity.	8
	Implement the conditional forwarding logic based on the forward_to configuration The comment suggests a conditional forwarding behavior based on the forward_to configuration. Implement this conditional logic to either forward the transaction directly or process it locally based on the configuration. src/eth/rpc/rpc_server.rs [383] -//TODO if the forward_to is up, dont call executor, just forward it +if ctx.config.forward_to.is_some() { + ctx.relayer.forward(transaction).await?; +} else { + let hash = transaction.hash; + match ctx.executor.transact(transaction).await { + // result is success + Ok(_) => (), + Err(e) => return Err(e), + } +} Suggestion importance[1-10]: 8 Why: This suggestion provides a clear and practical implementation for the conditional forwarding logic, improving the flexibility and functionality of the code.	8
Possible issue	Ensure that the removal of the storage parameter does not unintentionally affect system functionality The method init in IntegratedRelayerConfig no longer accepts a storage parameter, which was previously used to create a TransactionRelayer. Ensure that removing this dependency does not affect other parts of the system that might rely on storage being part of the TransactionRelayer. src/config.rs [335] -pub async fn init(&self) -> anyhow::Result<Option<Arc<TransactionRelayer>>> +pub async fn init(&self, storage: Arc<StratusStorage>) -> anyhow::Result<Option<Arc<TransactionRelayer>>> Suggestion importance[1-10]: 7 Why: This suggestion correctly identifies a potential issue with removing the storage parameter, which could affect other parts of the system. However, it does not provide a concrete solution, just a cautionary note.	7