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(fw): Remove _make_request and replace using it to EthRPC methods #568

Merged
merged 11 commits into from
Jun 14, 2024

Conversation

artemd24
Copy link
Contributor

@artemd24 artemd24 commented May 22, 2024

🗒️ Description

Remove _make_request from RequestManager in cli.gentest.py.
Replace using _make_request to next EthRPC methods: get_block_by_number, get_transaction_by_hash, debug_trace_call.
Add 2 more new methods to EthRPC: get_transaction_by_hash, debug_trace_call.

Extend EthRPC add new methods:

  1. get_transaction_by_hash(self, transaction_hash: str)
  2. debug_trace_call(self, tr: RequestManager.RemoteTransaction)

🔗 Related Issues

Resolve #561

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.

@artemd24 artemd24 changed the title Resolve #561: Remove _make_request and replace to EthRPC.post_request Resolve #561: Remove _make_request and replace using it to EthRPC.post_request May 22, 2024
@artemd24 artemd24 changed the title Resolve #561: Remove _make_request and replace using it to EthRPC.post_request feat(fw): Remove _make_request and replace using it to EthRPC.post_request May 22, 2024
@artemd24 artemd24 changed the title feat(fw): Remove _make_request and replace using it to EthRPC.post_request feat(fw): Remove _make_request and replace using it to EthRPC methods May 22, 2024
@artemd24 artemd24 force-pushed the main branch 2 times, most recently from 092c223 to b345f0f Compare May 22, 2024 17:01
@artemd24
Copy link
Contributor Author

Hello @danceratopz,
please verify changes

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Thanks a lot for getting on this!

Whilst reviewing your code, I realized that gentest is currently broken, so you wouldn't have been able to execute your code. I've addressed this in #576.

It might be worth waiting for #576 to get merged before making heavier changes here, but in the meantime, you can try to get gentest (in the #576 branch) running, for this you'll need access to an archive node. If you don't have one personally, you can get it running using a free endpoint from https://www.portal.grove.city, for example. Check gentest module docstring in #576 for more help.

Otherwise, we use tox to lint and type-check our code in order to enforce our code standards. Some of these are rather pedantic, but are generally very useful. Tox is executed as part of Github but you can run tox locally first to ensure that Github Actions pass. You I fixed up import ordering in 43f5e3f, but we still have a few other issues, which I've noted below. Feel free to ping me for help (danceratopz on telegram/discord, if you get stuck). Here's some help to get started with:

src/ethereum_test_tools/rpc/rpc.py Outdated Show resolved Hide resolved
@artemd24
Copy link
Contributor Author

@danceratopz
Thank you for the detailed comment!
I agree with you to wait until #576 will be merged
however I'm going to add some fixes from your comment, it looks like there are some work that has to be done

@artemd24 artemd24 reopened this Jun 1, 2024
Copy link
Collaborator

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

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

Some quick comments on my side! Nice one :D

docs/CHANGELOG.md Outdated Show resolved Hide resolved
src/cli/gentest.py Outdated Show resolved Hide resolved
src/cli/gentest.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/rpc/rpc.py Show resolved Hide resolved
src/ethereum_test_tools/rpc/rpc.py Show resolved Hide resolved
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Hey @artemd24, thanks again for this! I'll be afk for a while and didn't want to leave this hanging while away, so I tried this out today and made a few fixes - I hope you don't mind. I tried to split the commits logically, so you can treat them as a review. I also asked @spencer-tb for some quick feedback 🙂.

Feel free to pick up on @spencer-tb's comments and work on any of them them in a new PR! But don't feel obliged!

@danceratopz danceratopz merged commit cbe983f into ethereum:main Jun 14, 2024
5 checks passed
@artemd24
Copy link
Contributor Author

artemd24 commented Jun 14, 2024

@danceratopz @spencer-tb
Thank you very much for helping me! I was a bit stuck and didn't see where to go.
Anyway @danceratopz thank you for your patient and for refactoring I will certainly examine all changes!
Additional thanks to @spencer-tb, your comments are very useful for me in perspective of improvement of my involvement and contribution in opensource

spencer-tb pushed a commit that referenced this pull request Jun 26, 2024
Co-authored-by: Art <art@MacBook-Pro-Art.local>
Co-authored-by: danceratopz <danceratopz@gmail.com>
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/refactor: extend EthRPC framework class and use it in gentest.py
3 participants