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(tracing): extend Parity tracing VmExecutedOperation #3997

Merged

Conversation

pistomat
Copy link
Contributor

@pistomat pistomat commented Jul 31, 2023

Convert VmExecutedOperation::push from Option<> to Vec<> as per ethers-rs and erigon implementation

Closes #3883

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.

great, tysm for taking this on.

what's missing?

crates/rpc/rpc-types/src/eth/trace/parity.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #3997 (0d75998) into main (f41386d) will decrease coverage by 0.04%.
Report is 3 commits behind head on main.
The diff coverage is 0.00%.

Impacted file tree graph

Files Changed Coverage Δ
...revm/revm-inspectors/src/tracing/builder/parity.rs 0.00% <0.00%> (ø)
crates/rpc/rpc-types/src/eth/trace/parity.rs 66.31% <ø> (ø)

... and 11 files with indirect coverage changes

Flag Coverage Δ
integration-tests 16.33% <0.00%> (-0.01%) ⬇️
unit-tests 64.31% <0.00%> (-0.04%) ⬇️

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

Components Coverage Δ
reth binary 26.67% <ø> (ø)
blockchain tree 83.04% <ø> (ø)
pipeline 89.82% <ø> (ø)
storage (db) 74.30% <ø> (ø)
trie 94.70% <ø> (ø)
txpool 45.40% <ø> (-0.61%) ⬇️
networking 77.62% <ø> (-0.04%) ⬇️
rpc 58.51% <ø> (+0.04%) ⬆️
consensus 63.51% <ø> (ø)
revm 33.08% <0.00%> (ø)
payload builder 6.58% <ø> (ø)
primitives 87.92% <ø> (+<0.01%) ⬆️

@pistomat
Copy link
Contributor Author

I am also looking into VMOperation and members op and idx.

Ethers-rs has only op and it as an enum of OpCodes, while erigon has both op and idx.

I wanted to fix this in one PR, but can make another if you like.

@pistomat pistomat marked this pull request as ready for review July 31, 2023 12:22
@pistomat pistomat requested a review from Rjected as a code owner July 31, 2023 12:22
@pistomat
Copy link
Contributor Author

Ok, adding the op and idx is work for another time, apperently vmTrace is being deprecated on Alchemy and other providers, so seems like there is not much use for this anyway.

@onbjerg onbjerg added C-bug An unexpected or incorrect behavior A-rpc Related to the RPC implementation M-changelog This change should be included in the changelog labels Jul 31, 2023
@onbjerg
Copy link
Member

onbjerg commented Jul 31, 2023

@pistomat If it's work for another time, would you mind creating a new issue for that with a description of what is needed?

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.

lgtm

@mattsse mattsse added this pull request to the merge queue Jul 31, 2023
Merged via the queue into paradigmxyz:main with commit 3118e27 Jul 31, 2023
23 checks passed
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 C-bug An unexpected or incorrect behavior M-changelog This change should be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

trace_call VmTrace specification
3 participants