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.

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

Jump to bottom

fix: remove Clone from nonce-related APIs #1491

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: remove Clone from nonce-related APIs #1491

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e38f4c
fix: remove clone from nonce
Daniel-Aaron-Bloom Oct 16, 2024
cf173dc
Merge branch 'main' into dbloom/remove-clone
Daniel-Aaron-Bloom Oct 16, 2024
d64732e
Merge branch 'main' into dbloom/remove-clone
Daniel-Aaron-Bloom Nov 1, 2024
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
40 changes: 37 additions & 3 deletions crates/provider/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub trait ProviderLayer<P: Provider<T, N>, T: Transport + Clone, N: Network = Et
type Provider: Provider<T, N>;

/// Wrap the given provider in the layer's provider.
fn layer(&self, inner: P) -> Self::Provider;
fn layer(self, inner: P) -> Self::Provider;
}

/// An identity layer that does nothing.
Expand Down Expand Up @@ -57,6 +57,19 @@ where
}
}

impl<'a, P, T, N> ProviderLayer<P, T, N> for &'a Identity
where
T: Transport + Clone,
N: Network,
P: Provider<T, N>,
{
type Provider = P;

fn layer(self, inner: P) -> Self::Provider {
inner
}
}

impl<P, T, N> ProviderLayer<P, T, N> for Identity
where
T: Transport + Clone,
Expand All @@ -65,7 +78,7 @@ where
{
type Provider = P;

fn layer(&self, inner: P) -> Self::Provider {
fn layer(self, inner: P) -> Self::Provider {
inner
}
}
Expand All @@ -84,6 +97,27 @@ impl<Inner, Outer> Stack<Inner, Outer> {
}
}

impl<'a, P, T, N, Inner, Outer> ProviderLayer<P, T, N> for &'a Stack<Inner, Outer>
where
T: Transport + Clone,
N: Network,
P: Provider<T, N>,
&'a Inner: ProviderLayer<P, T, N>,
&'a Outer: ProviderLayer<<&'a Inner as ProviderLayer<P, T, N>>::Provider, T, N>,
{
type Provider = <&'a Outer as ProviderLayer<
<&'a Inner as ProviderLayer<P, T, N>>::Provider,
T,
N,
>>::Provider;

fn layer(self, provider: P) -> Self::Provider {
let inner = self.inner.layer(provider);

self.outer.layer(inner)
}
}

impl<P, T, N, Inner, Outer> ProviderLayer<P, T, N> for Stack<Inner, Outer>
where
T: Transport + Clone,
Expand All @@ -94,7 +128,7 @@ where
{
type Provider = Outer::Provider;

fn layer(&self, provider: P) -> Self::Provider {
fn layer(self, provider: P) -> Self::Provider {
let inner = self.inner.layer(provider);

self.outer.layer(inner)
Expand Down
6 changes: 3 additions & 3 deletions crates/provider/src/fillers/join_fill.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use futures::try_join;
/// This struct is itself a [`TxFiller`], and can be nested to compose any number of fill layers.
///
/// [`TransactionRequest`]: alloy_rpc_types_eth::TransactionRequest
#[derive(Clone, Copy, Debug)]
#[derive(Debug)]
pub struct JoinFill<L, R> {
left: L,
right: R,
Expand Down Expand Up @@ -136,7 +136,7 @@ where
N: Network,
{
type Provider = FillProvider<Self, P, T, N>;
fn layer(&self, inner: P) -> Self::Provider {
FillProvider::new(inner, self.clone())
fn layer(self, inner: P) -> Self::Provider {
FillProvider::new(inner, self)
}
}
2 changes: 1 addition & 1 deletion crates/provider/src/fillers/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ impl FillerControlFlow {
/// - **Finished**: The filler has filled in all properties that it can fill. [`TxFiller::status`]
/// should return [`FillerControlFlow::Finished`].
#[doc(alias = "TransactionFiller")]
pub trait TxFiller<N: Network = Ethereum>: Clone + Send + Sync + std::fmt::Debug {
pub trait TxFiller<N: Network = Ethereum>: Sized + Send + Sync + std::fmt::Debug {
/// The properties that this filler retrieves from the RPC. to fill in the
/// TransactionRequest.
type Fillable: Send + Sync + 'static;
Expand Down
8 changes: 4 additions & 4 deletions crates/provider/src/fillers/nonce.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::sync::Arc;
/// A trait that determines the behavior of filling nonces.
#[cfg_attr(target_arch = "wasm32", async_trait(?Send))]
#[cfg_attr(not(target_arch = "wasm32"), async_trait)]
pub trait NonceManager: Clone + Send + Sync + std::fmt::Debug {
pub trait NonceManager: Send + Sync + std::fmt::Debug {
/// Get the next nonce for the given account.
async fn get_next_nonce<P, T, N>(&self, provider: &P, address: Address) -> TransportResult<u64>
where
Expand All @@ -29,7 +29,7 @@ pub trait NonceManager: Clone + Send + Sync + std::fmt::Debug {
/// Unlike [`CachedNonceManager`], this implementation does not store the transaction count locally,
/// which results in more frequent calls to the provider, but it is more resilient to chain
/// reorganizations.
#[derive(Clone, Debug, Default)]
#[derive(Debug, Default)]
#[non_exhaustive]
pub struct SimpleNonceManager;

Expand All @@ -54,7 +54,7 @@ impl NonceManager for SimpleNonceManager {
///
/// There is also an alternative implementation [`SimpleNonceManager`] that does not store the
/// transaction count locally.
#[derive(Clone, Debug, Default)]
#[derive(Debug, Default)]
pub struct CachedNonceManager {
nonces: DashMap<Address, Arc<Mutex<u64>>>,
Copy link
Member

Choose a reason for hiding this comment

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

I see, unsure how we missed this

can we instead replace this with an Arc mutex instead? not a fan of dashmap anyway

Copy link
Author

Choose a reason for hiding this comment

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

From a technical standpoint, yes, that would be possible. However, I would recommend against it as it's preserving the footgun for any external NonceManager implementations downstream crates may make.

Copy link
Author

Choose a reason for hiding this comment

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

Also you wouldn't need to Arc<Mutex<_>>, just Arc<DashMap<_, _>> would be sufficient.

Copy link
Member

@mattsse mattsse Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this change we could do immediately, not sure I fully get the other parts of the pr yet, so I suggest to submit that separately

Copy link
Author

Choose a reason for hiding this comment

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

The rest of the PR is just the minimal changes to fix all the errors from removing Clone.

This PR starts by removing Clone from CachedNonceManager, SimpleNonceManager, and NonceManager.

This means none of them fulfill TxFiller until we remove Clone from that (and replace it with Sized since it still needs that).

Doing that breaks the ProviderLayer implementation for JoinFill so we change that to move self instead of cloning &self.

That of course requires changing the signature of ProviderLayer::layer itself, which then requires changing all the impl ProviderLayers.

The duplicate &'a impls are just back-compat for anything which needs to call layer by reference.

}
Expand Down Expand Up @@ -116,7 +116,7 @@ impl NonceManager for CachedNonceManager {
/// # Ok(())
/// # }
/// ```
#[derive(Clone, Debug, Default)]
#[derive(Debug, Default)]
pub struct NonceFiller<M: NonceManager = SimpleNonceManager> {
nonce_manager: M,
}
Expand Down
15 changes: 14 additions & 1 deletion crates/provider/src/layers/anvil.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,27 @@ impl From<Anvil> for AnvilLayer {
}
}

impl<'a, P, T> ProviderLayer<P, T, Ethereum> for &'a AnvilLayer
where
P: Provider<T>,
T: Transport + Clone,
{
type Provider = AnvilProvider<P, T>;

fn layer(self, inner: P) -> Self::Provider {
let anvil = self.instance();
AnvilProvider::new(inner, anvil.clone())
}
}

impl<P, T> ProviderLayer<P, T, Ethereum> for AnvilLayer
where
P: Provider<T>,
T: Transport + Clone,
{
type Provider = AnvilProvider<P, T>;

fn layer(&self, inner: P) -> Self::Provider {
fn layer(self, inner: P) -> Self::Provider {
let anvil = self.instance();
AnvilProvider::new(inner, anvil.clone())
}
Expand Down
15 changes: 14 additions & 1 deletion crates/provider/src/layers/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@ impl CacheLayer {
}
}

impl<'a, P, T, N> ProviderLayer<P, T, N> for &'a CacheLayer
where
P: Provider<T, N>,
T: Transport + Clone,
N: Network,
{
type Provider = CacheProvider<P, T, N>;

fn layer(self, inner: P) -> Self::Provider {
CacheProvider::new(inner, self.cache())
}
}

impl<P, T, N> ProviderLayer<P, T, N> for CacheLayer
where
P: Provider<T, N>,
Expand All @@ -53,7 +66,7 @@ where
{
type Provider = CacheProvider<P, T, N>;

fn layer(&self, inner: P) -> Self::Provider {
fn layer(self, inner: P) -> Self::Provider {
CacheProvider::new(inner, self.cache())
}
}
Expand Down
20 changes: 19 additions & 1 deletion crates/provider/src/layers/chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,32 @@ impl From<NamedChain> for ChainLayer {
}
}

impl<'a, P, T> ProviderLayer<P, T, Ethereum> for &'a ChainLayer
where
P: Provider<T>,
T: Transport + Clone,
{
type Provider = P;

fn layer(self, inner: P) -> Self::Provider {
if !inner.client().is_local() {
if let Some(avg_block_time) = self.average_blocktime_hint() {
let poll_interval = avg_block_time.mul_f32(0.6);
inner.client().set_poll_interval(poll_interval);
}
}
inner
}
}

impl<P, T> ProviderLayer<P, T, Ethereum> for ChainLayer
where
P: Provider<T>,
T: Transport + Clone,
{
type Provider = P;

fn layer(&self, inner: P) -> Self::Provider {
fn layer(self, inner: P) -> Self::Provider {
if !inner.client().is_local() {
if let Some(avg_block_time) = self.average_blocktime_hint() {
let poll_interval = avg_block_time.mul_f32(0.6);
Expand Down