Skip to content

feat: add retry strategy to client #512

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.

Sign up for GitHub

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

Merged
lpahlavi merged 10 commits into from
Oct 30, 2025
Merged

feat: add retry strategy to client #512

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fc4b48c
Add retry strategy to client
lpahlavi Oct 27, 2025
5c10d97
Merge branch 'main' into lpahlavi/client-retry-strategy
lpahlavi Oct 28, 2025
71f815e
Add integration test
lpahlavi Oct 28, 2025
f408069
Clippy
lpahlavi Oct 28, 2025
3808ba4
Make `EvmRpcRetryPolicy` a trait
lpahlavi Oct 29, 2025
b568fd2
Clippy
lpahlavi Oct 29, 2025
cd578ff
Clippy
lpahlavi Oct 29, 2025
b5801a7
Rename `EvmRpcRetryPolicy` to `RetryPolicy`
lpahlavi Oct 30, 2025
9290835
Inline `IsTooFewCyclesError`
lpahlavi Oct 30, 2025
66b537e
Improve `RetryPolicy` documentation
lpahlavi Oct 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 67 additions & 3 deletions evm_rpc_client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@
#[cfg(not(target_arch = "wasm32"))]
pub mod fixtures;
mod request;
mod retry;
mod runtime;

use candid::{CandidType, Principal};
Expand All @@ -138,6 +139,8 @@ use request::{
SendRawTransactionRequest, SendRawTransactionRequestBuilder,
};
pub use request::{CandidResponseConverter, EvmRpcConfig};
use retry::EvmRpcResult;
pub use retry::EvmRpcRetryStrategy;
pub use runtime::{IcError, IcRuntime, Runtime};
use serde::de::DeserializeOwned;
use std::sync::Arc;
Expand Down Expand Up @@ -192,6 +195,7 @@ pub struct ClientConfig<R, C> {
rpc_config: Option<RpcConfig>,
rpc_services: RpcServices,
response_converter: C,
retry_strategy: EvmRpcRetryStrategy,
}

/// A [`ClientBuilder`] to create a [`EvmRpcClient`] with custom configuration.
Expand All @@ -217,6 +221,7 @@ impl<R> ClientBuilder<R, CandidResponseConverter> {
rpc_config: None,
rpc_services: RpcServices::EthMainnet(None),
response_converter: CandidResponseConverter,
retry_strategy: Default::default(),
},
}
}
Expand All @@ -234,6 +239,7 @@ impl<R, C> ClientBuilder<R, C> {
rpc_config: self.config.rpc_config,
rpc_services: self.config.rpc_services,
response_converter: self.config.response_converter,
retry_strategy: self.config.retry_strategy,
},
}
}
Expand Down Expand Up @@ -278,6 +284,7 @@ impl<R, C> ClientBuilder<R, C> {
rpc_config: self.config.rpc_config,
rpc_services: self.config.rpc_services,
response_converter: AlloyResponseConverter,
retry_strategy: self.config.retry_strategy,
},
}
}
Expand All @@ -291,10 +298,17 @@ impl<R, C> ClientBuilder<R, C> {
rpc_config: self.config.rpc_config,
rpc_services: self.config.rpc_services,
response_converter: CandidResponseConverter,
retry_strategy: self.config.retry_strategy,
},
}
}

/// Mutates the builder to use the given retry strategy.
pub fn with_retry_strategy(mut self, retry_strategy: EvmRpcRetryStrategy) -> Self {
self.config.retry_strategy = retry_strategy;
self
}

/// Creates a [`EvmRpcClient`] from the configuration specified in the [`ClientBuilder`].
pub fn build(self) -> EvmRpcClient<R, C> {
EvmRpcClient {
Expand Down Expand Up @@ -763,15 +777,16 @@ impl<R, C: EvmRpcResponseConverter> EvmRpcClient<R, C> {
}
}

impl<R: Runtime, C> EvmRpcClient<R, C> {
impl<Runtime: runtime::Runtime, Converter> EvmRpcClient<Runtime, Converter> {
async fn execute_request<Config, Params, CandidOutput, Output>(
&self,
request: Request<Config, Params, CandidOutput, Output>,
) -> Output
where
Config: CandidType + Send,
Params: CandidType + Send,
Config: CandidType + Clone + Send,
Copy link
Contributor Author

@lpahlavi lpahlavi Oct 28, 2025

Choose a reason for hiding this comment

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

The Clone bounds are needed here to be able to clone the request when retrying.

Params: CandidType + Clone + Send,
CandidOutput: Into<Output> + CandidType + DeserializeOwned,
Output: EvmRpcResult,
{
let rpc_method = request.endpoint.rpc_method();
self.try_execute_request(request)
Expand All @@ -783,10 +798,59 @@ impl<R: Runtime, C> EvmRpcClient<R, C> {
&self,
request: Request<Config, Params, CandidOutput, Output>,
) -> Result<Output, IcError>
where
Config: CandidType + Clone + Send,
Params: CandidType + Clone + Send,
CandidOutput: Into<Output> + CandidType + DeserializeOwned,
Output: EvmRpcResult,
{
fn retry_request<Config, Params, CandidOutput, Output>(
strategy: &EvmRpcRetryStrategy,
request: Request<Config, Params, CandidOutput, Output>,
result: &Output,
num_retries: u32,
) -> Option<Request<Config, Params, CandidOutput, Output>>
where
Output: EvmRpcResult,
{
match strategy {
EvmRpcRetryStrategy::NoRetry => None,
EvmRpcRetryStrategy::DoubleCycles { max_num_retries } => {
if num_retries < *max_num_retries && result.is_too_few_cycles_error() {
let request = Request {
cycles: request.cycles.saturating_mul(2),
..request
};
Some(request)
} else {
None
}
}
}
}

let mut request = request;
let mut num_retries = 0;
loop {
let result = self.try_execute_request_once(request.clone()).await?;
request =
match retry_request(&self.config.retry_strategy, request, &result, num_retries) {
Some(request) => request,
None => return Ok(result),
};
num_retries += 1;
}
}

async fn try_execute_request_once<Config, Params, CandidOutput, Output>(
&self,
request: Request<Config, Params, CandidOutput, Output>,
) -> Result<Output, IcError>
where
Config: CandidType + Send,
Params: CandidType + Send,
CandidOutput: Into<Output> + CandidType + DeserializeOwned,
Output: EvmRpcResult,
{
self.config
.runtime
Expand Down
11 changes: 7 additions & 4 deletions evm_rpc_client/src/request/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#[cfg(feature = "alloy")]
pub(crate) mod alloy;

use crate::retry::EvmRpcResult;
use crate::runtime::IcError;
use crate::{EvmRpcClient, Runtime};
use candid::CandidType;
Expand Down Expand Up @@ -497,9 +498,10 @@ impl<R: Runtime, Converter, Config, Params, CandidOutput, Output>
/// If the request was not successful.
pub async fn send(self) -> Output
where
Config: CandidType + Send,
Params: CandidType + Send,
Config: CandidType + Clone + Send,
Params: CandidType + Clone + Send,
CandidOutput: Into<Output> + CandidType + DeserializeOwned,
Output: EvmRpcResult,
{
self.client
.execute_request::<Config, Params, CandidOutput, Output>(self.request)
Expand All @@ -510,9 +512,10 @@ impl<R: Runtime, Converter, Config, Params, CandidOutput, Output>
/// either the request response or any error that occurs while sending the request.
pub async fn try_send(self) -> Result<Output, IcError>
where
Config: CandidType + Send,
Params: CandidType + Send,
Config: CandidType + Clone + Send,
Params: CandidType + Clone + Send,
CandidOutput: Into<Output> + CandidType + DeserializeOwned,
Output: EvmRpcResult,
{
self.client
.try_execute_request::<Config, Params, CandidOutput, Output>(self.request)
Expand Down
43 changes: 43 additions & 0 deletions evm_rpc_client/src/retry/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
use evm_rpc_types::{MultiRpcResult, ProviderError, RpcError, RpcResult};

/// The retry strategy when performing calls to the EVM RPC canister.
#[derive(Clone, Eq, PartialEq, Debug, Default)]
pub enum EvmRpcRetryStrategy {
Copy link
Contributor Author

@lpahlavi lpahlavi Oct 28, 2025

Choose a reason for hiding this comment

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

I hesitated here to make this a trait instead of an enum, so users can provide their own retry implementation. In the end, I felt like it might have been over-engineered though and an enum might be sufficient. WDYT?

Copy link
Contributor

@gregorydemay gregorydemay Oct 29, 2025

Choose a reason for hiding this comment

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

One problem I see is that if we need some additional trait bounds for a new variant in EvmRpcRetryStrategy, then this will trickle down to all methods, such as try_execute_request, even if the caller would not use this particular variant. For that reason, my impression is that having a trait here would be useful. Something like in the spirit of tower::retry::Policy. One benefit is that the trait EvmRpcResult would also I think no longer be necessary, because the bound would be on the strategy implementing RetryPolicy. Example

pub trait RetryPolicy<Config, Params, CandidOutput, Output> {
    fn retry(
        &mut self, //can mutate self in case the strategy is stateful
        request: Request<Config, Params, CandidOutput, Output>, //take ownership, no separate_method `clone_request
        result: &mut Result<Output, IcError>, //strategy can decide to change result
    ) -> Option<Request<Config, Params, CandidOutput, Output>>;
}

WDYT?

Copy link
Contributor Author

@lpahlavi lpahlavi Oct 29, 2025

Choose a reason for hiding this comment

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

I actually originally had something very similar, and then moved to an enum... But I think you make some good points and in the end the more complicated solution is indeed probably better.

Note that I did add a clone_request method, otherwise, even with a NoRetry policy, requests would still need to be cloneable. This means we effectively have something extremely similar to tower::retry::Policy, and so I'm wondering if in the end it might not make more sense to just use that? We could use tower for the whole thing which would just require adding a 'static bound on the Config and Params types. WDYT?

Copy link
Contributor

@gregorydemay gregorydemay Oct 30, 2025

Choose a reason for hiding this comment

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

Thanks for the changes, the trait looks very good to me!

and so I'm wondering if in the end it might not make more sense to just use that?

For now I would not, for the following reasons:

  1. less dependencies
  2. simpler, because the current retry policy does not need to be async.

/// Do not perform any retries.
#[default]
NoRetry,

/// If a call fails due to insufficient cycles, try again with double the cycles until the
/// maximum number of allowed retries has been performed.
DoubleCycles {
/// The maximum number of retries to perform.
max_num_retries: u32,
},
}

pub trait EvmRpcResult {
fn is_too_few_cycles_error(&self) -> bool;
}

impl<T> EvmRpcResult for MultiRpcResult<T> {
fn is_too_few_cycles_error(&self) -> bool {
match self {
MultiRpcResult::Consistent(result) => result.is_too_few_cycles_error(),
MultiRpcResult::Inconsistent(results) => results
.iter()
.any(|(_, result)| result.is_too_few_cycles_error()),
}
}
}

impl<T> EvmRpcResult for RpcResult<T> {
fn is_too_few_cycles_error(&self) -> bool {
match self {
Err(err) => matches!(
err,
RpcError::ProviderError(ProviderError::TooFewCycles { .. })
),
_ => false,
}
}
}