Skip to content

Conversation

LeoPatOZ
Copy link
Collaborator

@LeoPatOZ LeoPatOZ commented Oct 20, 2025

Resolves #112

  1. Do we want to use any other features of backoff here?
  2. Is this sufficient logging?

Comment on lines 121 to 129
pub async fn subscribe_blocks(
&self,
) -> Result<Subscription<N::HeaderResponse>, RpcError<TransportErrorKind>> {
let provider = self.provider.clone();
self.retry_with_timeout(|| async { provider.subscribe_blocks().await }).await
}

#[allow(clippy::missing_errors_doc)]
pub(crate) async fn retry_with_timeout<T, F, Fut>(
Copy link
Collaborator

@0xNeshi 0xNeshi Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: instead of relying on closures, you could implement a local trait extension on all F: Fn() -> Fut, potentially avoiding provider clones, something like the following pseudocode:

trait RetryableWithTimeout<T, Fut> {
    async fn retry_with_timeout(&self) -> Result<T, RpcError<TransportErrorKind>>;
}

impl<T, Fut> RetryableWithTimeout<T, Fut> for F
where
    F: Fn() -> Fut
    Fut: Future<Output = Result<T, RpcError<TransportErrorKind>>>,
{
    //...
}

@LeoPatOZ LeoPatOZ marked this pull request as ready for review October 21, 2025 14:14
@LeoPatOZ LeoPatOZ requested a review from pepebndc as a code owner October 21, 2025 14:14
Copy link
Collaborator

@0xNeshi 0xNeshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice API, will be useful for multi-provider support (see #6 ).
Left comments mostly related to refactors and nits 👍

Comment on lines 8 to 28
//! ```rust,no_run
//! use alloy::{
//! network::Ethereum,
//! providers::{RootProvider, WsConnect},
//! rpc::client::ClientBuilder,
//! };
//! use event_scanner::safe_provider::SafeProvider;
//! use std::time::Duration;
//!
//! async fn example() -> Result<(), Box<dyn std::error::Error>> {
//! let provider = RootProvider::<Ethereum>::new(
//! ClientBuilder::default().ws(WsConnect::new("wss://localhost:8000")).await?,
//! );
//! let safe_provider = SafeProvider::new(provider)
//! .with_max_timeout(Duration::from_secs(30))
//! .with_max_retries(5);
//!
//! let block = safe_provider.get_block_by_number(12345.into()).await?;
//! Ok(())
//! }
//! ```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This should be marked to run in doc tests, to ensure it compiles (otherwise we might forget to change it when API changes).
  2. You can prefix import lines with # to hide them from previews, while still passing doc tests. See example below:
    Code:
image

Preview:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay very interesting
Took me a while to realise the doc comment has to be above the actual object (ofc..)

Also i adding no run still compiles the test https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html?highlight=no_run#attributes it just doesnt 'run' the code. Im not sure whats better but since we dont actually want to connect to a provider for the doc test we can leave it to no run

lmk what you think

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually removed the doc completely - i dont think we should expose the safe provider to outside the crate, therefore it doesnt make sense

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.

feat: External RPC Calls Should be Wrapped in Retry + Backoff Mechanisms

2 participants