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 tracing methods again #2851

Merged
merged 8 commits into from
Mar 27, 2023
Merged

Add tracing methods again #2851

merged 8 commits into from
Mar 27, 2023

Conversation

Uxio0
Copy link
Contributor

@Uxio0 Uxio0 commented Feb 27, 2023

What was wrong?

Closes #2830

How was it fixed?

Add tracing methods again

Todo:

Cute animal picture

@kclowes
Copy link
Collaborator

kclowes commented Feb 27, 2023

Thanks @Uxio0! We're doing a code freeze for v6 while we test out this last beta version, but will get this in as soon as we're stable. We're hoping we'll be able to release stable next week, but it depends on what sorts of bugs we find in the next few days.

@Uxio0
Copy link
Contributor Author

Uxio0 commented Feb 27, 2023

Sounds good, thanks @kclowes

Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

Thanks @Uxio0. I agree these are useful and it sits much nicer in a tracing module. v6 is out now so this can probably start inching its way back in. I just made a quick pass and added some general cleanup notes / nits. Would be good to add some tests before we merge this in. @kclowes I can try to contribute here this week and see if we can pull some from the old Parity module code (though that might take a bit to set up proper tests).

web3/types.py Outdated Show resolved Hide resolved
web3/types.py Outdated Show resolved Hide resolved
web3/tracing.py Outdated Show resolved Hide resolved
@Uxio0
Copy link
Contributor Author

Uxio0 commented Mar 23, 2023

Thanks @Uxio0. I agree these are useful and it sits much nicer in a tracing module. v6 is out now so this can probably start inching its way back in. I just made a quick pass and added some general cleanup notes / nits. Would be good to add some tests before we merge this in. @kclowes I can try to contribute here this week and see if we can pull some from the old Parity module code (though that might take a bit to set up proper tests).

Hi @fselmo , thanks for your review. I've fixed the issues you reported.

Regarding testing, I couldn't find any previous tests for this

@fselmo
Copy link
Collaborator

fselmo commented Mar 23, 2023

Regarding testing, I couldn't find any previous tests for this

Yeah at some point we had a fairly convoluted parity test fixture that was removed likely before all this got removed. I wonder if we had some related tests there but it wouldn't make much sense to bring it back in that same way. I'll try to think of a lightweight way we can add a test here to know if something breaks or not.

@kclowes and I were thinking that for now we might just want to manually test all the methods, merge it in if it looks good, and create an issue to track the testing for the tracing module 🤔.

@Uxio0
Copy link
Contributor Author

Uxio0 commented Mar 24, 2023

@kclowes and I were thinking that for now we might just want to manually test all the methods, merge it in if it looks good, and create an issue to track the testing for the tracing module 🤔.

Looks good to me

@fselmo
Copy link
Collaborator

fselmo commented Mar 24, 2023

rebased with master and force pushed... adding some formatters to these methods for human-readable values and re-testing

@fselmo fselmo force-pushed the add-tracing-methods branch 2 times, most recently from 06a0e6e to 3cd5c69 Compare March 24, 2023 22:11
@fselmo
Copy link
Collaborator

fselmo commented Mar 24, 2023

@kclowes @pacrob @Uxio0 I added formatters to these methods and tested all but the trace_rawTransaction but it looks like the output (here) is similar to trace_call (here) so I think we should be good there. Ready for final review. Bookkeeping issue created to add some tests for the tracing module (#2894).

- Add formatters for ``tracing`` module methods for human-readable values.
- Make the ``tracing`` module a default module on the ``Web3`` class instance.
- Remove `maxFeePerGas` and `maxPriorityFeePerGas` request formatters from `method_formatters.py` and add them in `rpc_abi.py` along with the rest of the transaction request formatters for consistency. This separation just causes confusion.
- Use ``TRANSACTION_PARAMS_ABIS`` formatters for ``trace_call``.
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

lgtm!

@fselmo fselmo merged commit 92409a0 into ethereum:master Mar 27, 2023
@kclowes
Copy link
Collaborator

kclowes commented Apr 5, 2023

@Uxio0 This has been released in v6.1.0. Thanks again!

@Uxio0
Copy link
Contributor Author

Uxio0 commented Apr 5, 2023

Awesome, thank you very much!

@Uxio0 Uxio0 deleted the add-tracing-methods branch April 25, 2023 12:51
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.

Parity tracing should be renamed and not removed
4 participants