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: add trace_replayBlockTransactions #2141

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

chirag-bgh
Copy link
Contributor

Closes #2010

Implements trace_replayBlockTransactions

Comment on lines 168 to 172
pub async fn replay_block_transactions(
&self,
block_id: BlockId,
trace_types: HashSet<TraceType>,
) -> Result<Option<Vec<TraceResultsWithTransactionHash>>> {
Copy link
Collaborator

@mattsse mattsse Apr 6, 2023

Choose a reason for hiding this comment

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

this is basically the same thing as trace_block, but the recorded traces differ:

https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/trace.rs#L238-L241

what we should do here is:

Convert trace_block into a helper function that can also unify replay_block_transactions. This new function should accept a TracingInspectorConfig and a callback function Fn() like this:

https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/eth/api/transactions.rs#L136-L140

so both trace block functions call this new helper functions but with a different config and callback (that converts the inspector into traces)

does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay
Yes this make sense

@mattsse mattsse added the A-rpc Related to the RPC implementation label Apr 6, 2023
@chirag-bgh chirag-bgh marked this pull request as draft April 6, 2023 10:41
@mattsse
Copy link
Collaborator

mattsse commented Apr 11, 2023

hey @chirag-bgh
I'd like to get this PR over the line,
Do you have enough bandwith to finish, or do you need pointers on the requested changes?

@gakonst
Copy link
Member

gakonst commented Apr 12, 2023

Bump @chirag-bgh - we're sprinting towards release so would love to merge this asap. If you cannot prioritize it's all good! We can take it over, thank you for getting the ball rolling..

@chirag-bgh
Copy link
Contributor Author

Bump @chirag-bgh - we're sprinting towards release so would love to merge this asap. If you cannot prioritize it's all good! We can take it over, thank you for getting the ball rolling..

I'm very sorry for keeping this stale. You can take it over!

@mattsse mattsse marked this pull request as ready for review April 12, 2023 09:11
@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2023

Codecov Report

Merging #2141 (4b5d807) into main (e87960e) will increase coverage by 0.02%.
The diff coverage is 65.30%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2141      +/-   ##
==========================================
+ Coverage   71.93%   71.96%   +0.02%     
==========================================
  Files         462      462              
  Lines       55617    55654      +37     
==========================================
+ Hits        40008    40049      +41     
+ Misses      15609    15605       -4     
Flag Coverage Δ
integration-tests 18.80% <65.30%> (+0.03%) ⬆️
unit-tests 66.81% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
crates/rpc/rpc/src/trace.rs 47.69% <65.30%> (+4.38%) ⬆️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mattsse mattsse merged commit 554c825 into paradigmxyz:main Apr 12, 2023
Rjected pushed a commit that referenced this pull request Apr 13, 2023
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Related to the RPC implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement trace_replayTransaction + trace_replayBlockTransactions`
4 participants