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

fix: remove Clone from nonce-related APIs #1491

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

Conversation

Daniel-Aaron-Bloom
Copy link

This is technically a breaking change as it modifies the public signatures of multiple traits and structs (removing clone, switching to self by value instead of reference), but it should have minimal impact on most downstream users.

Motivation

tl;dr is cloning a NonceManager (particularly CachedNonceManager because DashMap is not an Arc) is a big foot gun. It's actually even worse than that because the various layers automatically clone stuff on your behalf. The why is because cloning often turns numbers that should only be used once into numbers that are used twice.

Solution

Remove Clone from the relevant APIs, and patch up other APIs to remove their dependency on clonability of TxFillers. This does result in some duplicate ProviderLayer impls (by-ref and by-val).

PR Checklist

  • Added Tests Not applicable
  • Added Documentation Not applicable
  • Breaking changes

@@ -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

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.

@mattsse mattsse mentioned this pull request Nov 6, 2024
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.

2 participants