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

Add transparent multicall middleware #2684

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

yorhodes
Copy link

@yorhodes yorhodes commented Nov 22, 2023

Motivation

Concurrent eth_calls are expensive and require many network round trips. Large codebases are difficult to refactor to leverage Multicall contract abstraction.

Fixes #2508

Solution

Introduces MulticallMiddleware that maintains a queue of call requests and callbacks. Leverages existing Multicall contract to batch many calls into a single RPC request. Uses tokio mpsc and oneshot pattern for async queueing.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@yorhodes yorhodes changed the title Add skeleton of multicall middleware Add transparent multicall middleware Nov 22, 2023
@yorhodes yorhodes marked this pull request as ready for review November 23, 2023 05:44
@yorhodes
Copy link
Author

hopefully more in scope than my last middleware contribution #2398

Copy link
Collaborator

@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 late review.

I think this looks pretty neat and I can see how this is useful

a few small requests.

design wise, I'm not sure we need a new service for this if we instead put the MultiCall handling into a shared state via some async Mutex, we could probably make it work like that, but I guess a service task also works fine

I think one perf issue here is that executing multicalls is now sequential and no longer concurrent which could be an issue

ethers-middleware/src/multicall.rs Show resolved Hide resolved
ethers-middleware/src/multicall.rs Outdated Show resolved Hide resolved
ethers-providers/src/rpc/transports/mock.rs Outdated Show resolved Hide resolved
ethers-middleware/src/multicall.rs Outdated Show resolved Hide resolved
ethers-middleware/src/multicall.rs Outdated Show resolved Hide resolved
ethers-middleware/src/multicall.rs Outdated Show resolved Hide resolved
ethers-middleware/src/multicall.rs Outdated Show resolved Hide resolved
Comment on lines +109 to +110
let (calls, callbacks): (Vec<_>, Vec<_>) = requests.into_iter().unzip();

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need any special handling if there's only one request?

Copy link
Author

Choose a reason for hiding this comment

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

we don't need special handling but its a minor optimization?

@yorhodes
Copy link
Author

design wise, I'm not sure we need a new service for this if we instead put the MultiCall handling into a shared state via some async Mutex, we could probably make it work like that, but I guess a service task also works fine

@mattsse I agree ideally users dont need to think about another service
I'm not actually sure how to achieve this because I don't have great knowledge of the available async primitives but if you give me some guidance I can try refactoring

I think one perf issue here is that executing multicalls is now sequential and no longer concurrent which could be an issue

I'm not sure how we can avoid this and achieve the transparent functionality? I assume anyone who wants to use the middleware would like to eliminate the concurrency on the RPC

@yorhodes
Copy link
Author

yorhodes commented Nov 30, 2023

for posterity, @mattsse and I discussed offline and decided to punt on some parking_lot::Mutex approach that does not require an additional service task for a followup PR

Copy link
Collaborator

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

last pedantic style nits.

I can see how this can be useful.

Since this only adds an additional multicall solution I think we can include this.

ethers-middleware/src/multicall.rs Outdated Show resolved Hide resolved
ethers-middleware/src/multicall.rs Outdated Show resolved Hide resolved
yorhodes and others added 2 commits December 4, 2023 11:56
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@BlinkyStitt
Copy link
Contributor

BlinkyStitt commented Feb 6, 2024

This looks much more ergonomic. Very neat idea.

I don't like that you have to register all the abi ahead of time though. I don't always know the abis at start but I'd still like to have the calls batched still.

Also, having to register and check the abis like this has a potential for hash collisions.

And how does calling for blocks other than latest work? I see some possibly related comments about system transactions but need to read more of the code to tell what those are.

@yorhodes
Copy link
Author

yorhodes commented Feb 6, 2024

This looks much more ergonomic. Very neat idea.

Thanks! Seems related to your work on https://github.com/llamanodes/web3-proxy

I don't like that you have to register all the abi ahead of time though. I don't always know the abis at start but I'd still like to have the calls batched still. Also, having to register and check the abis like this has a potential for hash collisions.

Knowing the ABIs is necessary to leverage multicall because you need to decode and slice the return data. Collisions are not handled but a more robust implementation could register ABIs for specific to addresses, rather than globally. This registration could be automated whenever a contract instance is connected to a provider with this middleware.

And how does calling for blocks other than latest work? I see some possibly related comments about system transactions but need to read more of the code to tell what those are.

This isnt supported but you could implement this by having separate queues per block number

@BlinkyStitt
Copy link
Contributor

I did some experimenting today combining some of these ideas with some non-transparent multicall code that I wrote years ago. So far it only handles call and not the other helpers, but it doesn't need abis and it supports setting the block.

setting the block i think is rather crucial. Otherwise the transparent nature of the middleware could lead to very surprising results.

I also want to automatically add state overrides for the contract if it's used on a block that predates the contract being deployed.

@BlinkyStitt
Copy link
Contributor

Here's a gist of what I did: https://gist.github.com/BlinkyStitt/837ffdbee2892c8857d192a5f4604b9c

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: automatically batch consecutive eth_calls with a multicall
3 participants