fix: timeout pending transaction in relayer

[carneiro-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: The PR includes error and warning logs which are good for monitoring the system's behavior in case of failures or delays. However, it's not clear if these logs are sufficient for all new branches of execution introduced. It would be beneficial to ensure that all error states and exceptional conditions are logged.
	Timeout Handling: The use of tokio::time::timeout is appropriate for handling potential delays in the receipt of transactions. However, the hardcoded timeout duration (30 seconds) might not be suitable for all environments or conditions. Consider making this configurable.
	Retry Logic: The changes in retry logic (*this.retries_remaining <= 0) are sensible, ensuring that the transaction does not endlessly retry. However, it's crucial to ensure that this logic correctly matches the intended behavior, especially in edge cases where retries might decrement in unexpected ways.

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add error handling for None case in substrate_receipt comparison Consider handling the case where substrate_receipt.compare(&stratus_receipt) might return an error due to substrate_receipt being None, which is not currently handled. src/eth/relayer/external.rs [157-163] -if let Err(compare_error) = substrate_receipt.compare(&stratus_receipt) { - let err_string = compare_error.to_string(); - let error = log_and_err!("transaction mismatch!").context(err_string.clone()); - self.save_mismatch(stratus_receipt, substrate_receipt, &err_string).await; - return error.map_err(RelayError::Mismatch); +if let Some(receipt) = substrate_receipt { + if let Err(compare_error) = receipt.compare(&stratus_receipt) { + let err_string = compare_error.to_string(); + let error = log_and_err!("transaction mismatch!").context(err_string.clone()); + self.save_mismatch(stratus_receipt, receipt, &err_string).await; + return error.map_err(RelayError::Mismatch); + } else { + return Ok(()); + } } else { - return Ok(()); + tracing::error!(?tx_hash, "substrate_receipt is None, cannot compare."); + return Err(RelayError::InvalidData(anyhow!("substrate_receipt is None"))); } Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug where substrate_receipt could be None, which is not currently handled. Adding this error handling improves the robustness of the code.	9
Maintainability	Replace hardcoded timeout duration with a configurable constant Replace the hardcoded timeout duration with a configurable value or constant to improve flexibility and maintainability. src/eth/relayer/external.rs [147] -timeout(Duration::from_secs(30), substrate_receipt).await +timeout(Duration::from_secs(TIMEOUT_DURATION), substrate_receipt).await Suggestion importance[1-10]: 8 Why: Using a configurable constant for the timeout duration improves maintainability and flexibility, allowing for easier adjustments in the future without modifying the code directly.	8
	Refactor nested match statements into separate functions for clarity Refactor the nested match statements to reduce complexity and improve code readability. src/eth/relayer/external.rs [155-176] match receipt { - Ok(Some(substrate_receipt)) => { - if let Err(compare_error) = substrate_receipt.compare(&stratus_receipt) { - let err_string = compare_error.to_string(); - let error = log_and_err!("transaction mismatch!").context(err_string.clone()); - self.save_mismatch(stratus_receipt, substrate_receipt, &err_string).await; - return error.map_err(RelayError::Mismatch); - } else { - return Ok(()); - } - } - Ok(None) => { - if start.elapsed().as_secs() <= 30 { - tracing::warn!(?tx_hash, "no receipt returned by substrate, retrying..."); - } else { - tracing::error!(?tx_hash, "no receipt returned by substrate for more than 30 seconds, retrying block"); - return Err(RelayError::CompareTimeout(anyhow!("no receipt returned by substrate for more than 30 seconds"))); - } - } + Ok(Some(receipt)) => process_receipt(receipt, &stratus_receipt, &tx_hash), + Ok(None) => handle_no_receipt(&start, &tx_hash), Err(error) => { tracing::error!(?tx_hash, ?error, "failed to fetch substrate receipt, retrying..."); } } +// Define `process_receipt` and `handle_no_receipt` elsewhere in your module + Suggestion importance[1-10]: 7 Why: Refactoring nested match statements into separate functions can improve code readability and maintainability, although it is not critical for functionality.	7
Best practice	Improve variable naming for better code clarity Use a more descriptive variable name than this to improve code readability and maintainability. src/infra/blockchain_client/pending_transaction.rs [90-91] -let this = self.project(); -tracing::debug!(?this.state, ?this.retries_remaining); +let current_transaction = self.project(); +tracing::debug!(?current_transaction.state, ?current_transaction.retries_remaining); Suggestion importance[1-10]: 6 Why: Using a more descriptive variable name enhances code readability and maintainability, but it is a minor improvement.	6