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(alloy-provider): add abstraction for NonceFiller behavior #1108

Merged
merged 6 commits into from
Aug 28, 2024

Conversation

StackOverflowExcept1on
Copy link
Contributor

Motivation

Resolves #1073

Solution

I added trait NonceManager and two implementations: SimpleNonceManager and CachedNonceManager to allow the user to choose the behavior of nonces filling. In the recommended fillers, I also changed the implementation from CachedNonceManager to SimpleNonceManager to make the recommended fillers more resilient to chain reorganizations. The issue was described in more detail in #1073 (comment).

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@zerosnacks
Copy link
Member

zerosnacks commented Aug 8, 2024

Good point on the re-org resistance. An alternative solution could be to have a function a user can call to refetch the nonce, overwriting the cache entry. Assuming they have a system that is re-org aware they can simply call this function.
I would prefer that over having two distinct implementations.

@StackOverflowExcept1on
Copy link
Contributor Author

@zerosnacks

An alternative solution could be to have a way of having a user be able to indicate that the nonce needs to be refetched.

Even if we indicate whether to fetch nonce from the rpc when sending the rpc request, it will still be inconvenient since you won't be able to fully customize the behavior as you wish.

@zerosnacks
Copy link
Member

Even if we indicate whether to fetch nonce from the rpc when sending the rpc request, it will still be inconvenient since you won't be able to fully customize the behavior as you wish.

Could you expand on that? What kind of behavior would you be customizing that is not covered by my proposed approach?

@StackOverflowExcept1on
Copy link
Contributor Author

@zerosnacks this will help implement any behavior, but on the caller side you will need to pass a boolean value or use some wrapper, which seems inconvenient on the caller side. We will also need wrappers for providers, alloy::contract, etc.

@StackOverflowExcept1on
Copy link
Contributor Author

Also consider that you can't just get a NonceFiller instance and indicate it to fetch rpc, since it might be wrapped with JoinFill. So you'll have to invent some mechanism to interact with the NonceFiller via interior mutability.

@zerosnacks
Copy link
Member

cc @onbjerg / @prestwich curious to hear your thoughts on this proposal / approach

@onbjerg
Copy link
Contributor

onbjerg commented Aug 8, 2024

It makes sense to have other types of nonce management strategies. You can already do this using a custom filler, but this simplifies implementing a new nonce strategy somewhat, so I think this is ok.

Noting that this is a breaking change, though

cc @mattsse

@onbjerg onbjerg added the enhancement New feature or request label Aug 8, 2024
@StackOverflowExcept1on
Copy link
Contributor Author

Hi, can someone take a look at the PR?

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

sorry for the delay @StackOverflowExcept1on ,

supportive, but have some nits and suggestions

crates/provider/src/fillers/nonce.rs Show resolved Hide resolved
crates/provider/src/fillers/nonce.rs Outdated Show resolved Hide resolved
crates/provider/src/builder.rs Show resolved Hide resolved
crates/provider/src/fillers/nonce.rs Outdated Show resolved Hide resolved
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

@mattsse mattsse merged commit ec70d9c into alloy-rs:main Aug 28, 2024
26 checks passed
@StackOverflowExcept1on StackOverflowExcept1on deleted the alloy-nonce-manager branch August 28, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] NonceFiller doesn't work after some contracts are deployed.
4 participants