-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
RFC: Add rpc method eth_callMany #4070
Conversation
Codecov Report
... and 18 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great, 1 question re error format and a few suggestions.
I pushed linting fixes to make the reviewing process easier
crates/rpc/rpc-types/src/eth/call.rs
Outdated
pub output: Option<Bytes>, | ||
/// eth_call error output | ||
pub error: Option<Bytes>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should match eth_call
?
is the error field also supposed to represent the error output of eth_call
? if so then this should be a String?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay! I can always do a err.to_string()
One question I do have is how does the RPC API handle the conversion from Error
to String
? E.g. specifically for the eth_call
method, the output is Result<Bytes, EthApiError>
, where and when does the program know to convert the EthApiError
to a String
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like this:
https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/eth/error.rs#L537-L547
which wraps the output in an error which formats it on .to_string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the error get formatted into a string? or is that handled by serde?
crates/rpc/rpc/src/eth/api/server.rs
Outdated
let res = self | ||
.on_blocking_task(|this| async move { | ||
this.call_many(bundle, state_context, state_override).await | ||
}) | ||
.await?; | ||
|
||
Ok(res) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call_many already spawns on the tracing pool and only does non-blocking async work otherwise, so we can call it directly and don't need to spawn a new blocking task here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referencing the call
function which also spawns on the tracing pool via transact_call_at -> spawn_with_call_at
. Is that intentional?
e6c4083
to
8ca27f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, last style nits, and 1 question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, lgtm
I only made one change re how the task is spawned
3d4f68b
to
17ff054
Compare
This PR adds in the
eth_callMany
RPC call.The PR adheres to the
eth_callMany
API outlined in erigon (erigontech/erigon#4471) but instead ofBundles (Vec<Bundle>)
it will only be aBundle
for simplicityNote: Not sure how to add tests, am not really familiar with Rust. Some guidance would be appreciated
How I manually tested it:
Output