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

geth.txpool async request #2273

Merged
merged 1 commit into from
Jan 7, 2022
Merged

Conversation

dbfreem
Copy link
Contributor

@dbfreem dbfreem commented Dec 22, 2021

What was wrong?

I wanted to go ahead and put this out here as a work in progress to get feedback on if this is the right path for starting to add async to the geth package.

Issue #1413

How was it fixed?

Todo:

@dbfreem
Copy link
Contributor Author

dbfreem commented Dec 27, 2021

Working on fixing the typing. Went back and forth on ignore typing for the async methods since the call the same method as the sync methods in tx_pool.py. The options I saw were to duplicate the method in tx_pool.py and change the return type or ignore the typing. I saw in eth.py the typing was ignored in this scenario so I went that route.

I Still need to look and see why linting is calling out typing in the testing module.

@dbfreem dbfreem marked this pull request as ready for review December 29, 2021 00:56
@dbfreem
Copy link
Contributor Author

dbfreem commented Dec 29, 2021

I had a question about test_txpool_content.py and test_txpool_inspect.py. I put the test for this PR in the Module testing as it seems like other async testing was placed there.

It seems like the test in test_txpool_content and test_txpool_inspect are always going to be skipped since they will always rollup to the web3 fixture in the root conftest.py and the EthereumTestProvider causes these test to be skipped by skip_if_testrpc. It seems like these unit test should be included in the module testing but I could be wrong here, just curious what I might be missing here.

@dbfreem
Copy link
Contributor Author

dbfreem commented Dec 29, 2021

This PR is ready for review

@kclowes
Copy link
Collaborator

kclowes commented Jan 5, 2022

Will take a look! Thank you!

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.

This looks good to me, thanks for taking it on! I just left one doc change, and one request for adding a test. Let me know what you think!

@kclowes
Copy link
Collaborator

kclowes commented Jan 6, 2022

I saw in eth.py the typing was ignored in this scenario so I went that route.

Thanks, that's the preferred approach for now. Hopefully we'll be able to figure out something a little nicer down the road.

It seems like the test in test_txpool_content and test_txpool_inspect are always going to be skipped since they will always rollup to the web3 fixture in the root conftest.py and the EthereumTestProvider causes these test to be skipped by skip_if_testrpc. It seems like these unit test should be included in the module testing but I could be wrong here, just curious what I might be missing here.

Yes, you're right that they will get skipped. I'm not sure why they're in the core tests. We can probably get rid of them, but I'll do that in another PR.

Thanks!

@dbfreem
Copy link
Contributor Author

dbfreem commented Jan 7, 2022

@kclowes I fixed those issues and this is ready for review

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.

Thank you @dbfreem! This looks good to me. I'll make a quick commit with the lint fix and some whitespace changes and then get this merged!

@kclowes kclowes force-pushed the feature/asyc_txpool branch from 06aced6 to 67443fc Compare January 7, 2022 19:37
@kclowes kclowes force-pushed the feature/asyc_txpool branch from 67443fc to c871e64 Compare January 7, 2022 19:39
@kclowes kclowes merged commit 6a90a26 into ethereum:master Jan 7, 2022
@dbfreem dbfreem deleted the feature/asyc_txpool branch July 9, 2022 10:17
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