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

✨ Added async eth.modify_transaction #2921

Merged
merged 12 commits into from
May 1, 2023
Merged

✨ Added async eth.modify_transaction #2921

merged 12 commits into from
May 1, 2023

Conversation

DavidRomanovizc
Copy link
Contributor

@DavidRomanovizc DavidRomanovizc commented Apr 17, 2023

What was wrong?

Related to Issue #2825

How was it fixed?

Added the modify_transaction method to the AsyncEth class, and adds corresponding tests.

Todo:

Cute Animal Picture

image

@DavidRomanovizc
Copy link
Contributor Author

Hi, @kclowes @fselmo, could you review my PR, please

@kclowes
Copy link
Collaborator

kclowes commented Apr 21, 2023

We've been busy with other things, but will take a look when we get a chance. Thanks for the PR!

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

It looks like you have some failing tests on CI. It wasn't running before for some reason. I also left a comment about re-using the existing extract_valid_transaction_params. Let me know if you don't have time/bandwidth to update this PR and one of us can take over. Thanks!

web3/_utils/async_transactions.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good!

@kclowes kclowes merged commit 201c49f into ethereum:main May 1, 2023
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.

2 participants