Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: external relayer (part 1) #879

Merged
merged 66 commits into from
May 29, 2024
Merged

feat: external relayer (part 1) #879

merged 66 commits into from
May 29, 2024

Conversation

carneiro-cw
Copy link
Contributor

WIP

@carneiro-cw
Copy link
Contributor Author

I will merge this PR since it is starting to conflict too much, and I will resume work on the relayer on a new PR

@carneiro-cw carneiro-cw marked this pull request as ready for review May 29, 2024 15:08
@carneiro-cw carneiro-cw requested a review from a team as a code owner May 29, 2024 15:08
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

4, because the PR introduces significant changes across multiple files including Rust and SQL, affecting core functionalities such as transaction relaying and mining. The complexity of the changes, especially with the introduction of external relayers and modifications to the mining configuration, requires careful review to ensure correctness and maintainability.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The new ExternalRelayer and ExternalRelayerClient classes are introduced without accompanying unit tests. This could lead to undetected bugs in relaying logic.

Performance Concern: The relay_next_block function in ExternalRelayer uses a SQL query that might not scale well with a large number of unrelayed blocks due to its subquery for the minimum block number.

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/relayer/external.rs
suggestion      

Consider implementing retry logic with exponential backoff for the relay_next_block method to handle transient failures in block relaying. This will enhance the robustness of the relaying process. [important]

relevant linepub async fn relay_next_block(&self) -> anyhow::Result> {

relevant filesrc/eth/relayer/external.rs
suggestion      

Add detailed logging for each step in the relay_dag function to improve traceability and debugging of the transaction relaying process. [important]

relevant lineasync fn relay_dag(&self, mut dag: TransactionDag) -> anyhow::Result<()> {

relevant filesrc/eth/block_miner.rs
suggestion      

Ensure error handling for relayer.send_to_relayer(block.clone()).await?; to manage scenarios where the relayer service might be down or unresponsive. Consider a fallback mechanism or queuing system. [important]

relevant linerelayer.send_to_relayer(block.clone()).await?;

relevant filesrc/config.rs
suggestion      

Validate configurations for ExternalRelayerClientConfig and ExternalRelayerServerConfig during startup to ensure all required parameters are provided and valid. This prevents runtime errors due to misconfiguration. [medium]

relevant linepub struct ExternalRelayerClientConfig {

@carneiro-cw carneiro-cw changed the title feat: external relayer feat: external relayer (part 1) May 29, 2024
Copy link

Failed to generate code suggestions for PR

@carneiro-cw carneiro-cw enabled auto-merge (squash) May 29, 2024 15:24
@carneiro-cw carneiro-cw merged commit ef048f8 into main May 29, 2024
31 checks passed
@carneiro-cw carneiro-cw deleted the external_relayer branch May 29, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants