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.

Sign up for GitHub

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

Jump to bottom

Feature/async geth personal #2299

Merged
merged 1 commit into from
Jan 31, 2022
Merged

Feature/async geth personal #2299

merged 1 commit into from
Jan 31, 2022

Conversation

dbfreem
Copy link
Contributor

@dbfreem dbfreem commented Jan 12, 2022 •
edited
Loading

What was wrong?

Added Async to Geth Personal

Related to Issue #1413

From a testing standpoint I used the GoEthereumPersonalModuleTest as a starting point. Since a lot of the functionality was tested in GoEthereumPersonalModuleTest I put bare minimum test on the Async side. I was trying not to have duplicates of all the test but there may be a better approach here.

Todo:

I had a couple of outstanding questions about adding Async to Geth Personal

  1. There are three methods that are not documented in web3.geth.rst so I couldn't add the links to them in the providers.rst
  2. I did not implement the deprecated methods in the AsyncGethPersonal class. This seemed to fit what had been done in Eth with the deprecated methods not being implemented in the async class.
  3. list_wallets and sign_typed_data do not seem to be implemented in the Geth Personal Namespace. Should they be in the methods for Geth Personal?
  4. I did not add a unit test for signed_typed_data since the test in the GoEthereumPersonalModuleTest class were marked as xfail. I can implement them if needed.
  5. Was thinking about changing the name of personal_module in module testing to go_ethereum_personal_module to align with the other Geth integration test but wanted to check first.
  6. How to handle the test for import_raw_key. I am currently going to remove it in GoEthereumAsyncPersonalModuleTest since it is failing because the key used in the test fixture is already imported. Should I generate a random key for testing in this test??

@dbfreem
Copy link
Contributor Author

dbfreem commented Jan 12, 2022 •
edited
Loading

I Need to look at the lint failures and test_personal_import_raw_key failures.

@dbfreem dbfreem marked this pull request as ready for review January 12, 2022 10:37
@dbfreem
Copy link
Contributor Author

dbfreem commented Jan 27, 2022

This is ready for review

pacrob
pacrob reviewed Jan 27, 2022
View reviewed changes
web3/geth.py Outdated Show resolved Hide resolved
pacrob
pacrob reviewed Jan 27, 2022
View reviewed changes
web3/geth.py Outdated Show resolved Hide resolved
pacrob
pacrob reviewed Jan 28, 2022
View reviewed changes
web3/geth.py Outdated Show resolved Hide resolved
@pacrob
Copy link
Contributor

pacrob commented Jan 28, 2022 •
edited
Loading

Thanks for all your work! On your questions:

  1. Noted.
  2. Yep, that's correct.
  3. list_wallets isn't in their docs but is in their code. sign_typed_data isn't supported now but is expected to in the future. They are good to stay in the code, but remove sign_typed_data from providers.rst for now.
  4. Please do add the xfail test so that we notice when it starts passing.
  5. Yes please! Sorry to ask you to update the names twice now, but it will be helpful when tracking things down.
  6. You can add constants THIRD_PRIVATE_KEY_HEX and associated address, following in line with those already used in the GoEthereumPersonalModuleTest. Feels a little clunky, but without being able to delete accounts it seems the best way.

pacrob
pacrob reviewed Jan 28, 2022
View reviewed changes
docs/providers.rst Outdated Show resolved Hide resolved
@dbfreem
Copy link
Contributor Author

dbfreem commented Jan 29, 2022

Hey @pacrob thanks for responding to my questions. I made the suggested changes. Let me know if you see anything else in this PR that needs to be modified.

pacrob
pacrob reviewed Jan 31, 2022
View reviewed changes
web3/geth.py Outdated Show resolved Hide resolved
@pacrob
Copy link
Contributor

pacrob commented Jan 31, 2022

A couple more spelling fixes, but otherwise looking good!

@dbfreem
Copy link
Contributor Author

dbfreem commented Jan 31, 2022

I promise I can spell haha, sorry about all the spelling errors in this PR.

kclowes
kclowes approved these changes Jan 31, 2022
View reviewed changes
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 @dbfreem! Looks good to me! I'll squash and merge!

Thanks for the review @pacrob!

@kclowes kclowes force-pushed the feature/async_geth_personal branch from 204cf1e to 93ffe5f Compare January 31, 2022 22:37
@kclowes kclowes merged commit e892434 into ethereum:master Jan 31, 2022
dsahdr added a commit to Rock-n-Block/web3.py that referenced this pull request Feb 15, 2022
@dsahdr @dbfreem
@alexpvpmindustry @fselmo @DefiDebauchery @miohtama @kclowes @mrshu @broper2 @calina-c @wiseaidev @wolovim @coccoinomane
Rebase (#2)
80dd4f0 
* move default_account and default_block properties and setters to BaseEth so Eth and AsyncEth can access

* Feature/ens request (ethereum#2319)

* fixed ens contract function called twice

* newsfragment

* small typo in documentation

* add newsfragment

* Only apply ``to_hexbytes`` formatter if value is not null

* asyncify eth.get_logs (ethereum#2310)

* asyncify eth.get_logs

* factor out `assert_contains_log` function

Co-authored-by: Paul Robinson <paul@pacrob.com>

* Add Github link to the main doc landing

Because Github link is extremely useful

* Newsfragment for github link to docs

* Update typing extensions to allow v4 (ethereum#2217)

* Update typing extensions to allow v4

* Loosen typing-extensions version

* Add newsfragment for typing-extensions bump

* Try out new py-evm requirements in eth-tester

* Remove xfails for newly passing eth-tester tests

* Upgrade eth-account requirement

* Add newsfragment for eth-tester bump

* correct misspellings and update referenced geth version

* Compile release notes

* Bump version: 5.26.0 → 5.27.0

* Add Async Geth Personal module (ethereum#2299)

* fix: Missing commas (ethereum#2327)

* fix: Missing commas

* Add newsfragment for exception retry middleware whitelist

Co-authored-by: kclowes <kclowes@users.noreply.github.com>

* Fixes ethereum#2259, remove dependency on eth_maxPriorityFeePerGas

* fix lint and integration tests

* refactor: utility for estimating maxPriorityFeePerGas via eth_feeHistory

Refactor idea from PR ethereum#2259 into sync and async fee utility methods. Change params passed into eth_feeHistory to values that allowed for better results when we tested locally. Add a min and max to the estimated fee history so that we don't allow unsuspecting users to contribute to fee bloating. Max and min values keep the priority fee within a range that healthy blocks should accept, so these transactions would be accepted when fee prices settle from high-fee periods.

* add tests for max_priority_fee when eth_maxPriorityFeePerGas is not available

* asyncify eth.syncing

* formatting and validation middleware async support

* Properly test unused code in test

* Align NamedTuples (ethereum#2312)

* Align NamedTuples

* Add NamedTuple alignment test.

* Add newsfragment for NamedTuple change

Co-authored-by: kclowes <kclowes@users.noreply.github.com>

* rm ens.utils.dict_copy

Signed-off-by: Harmouch101 <mahmoudddharmouchhh@gmail.com>

* fixed lint error

Signed-off-by: Harmouch101 <mahmoudddharmouchhh@gmail.com>

* Update main.py

* add newsfragment

* Feature/async geth admin (ethereum#2329)

* Added BaseGethPersonal to geth.py

* Added AsyncGethPersonal and test

* Added Docs

* lint fixes

* news fragment update

* removed import_raw_key test for now

* mypy typing issues

* typo

* Added AsyncGethAdmin and BaseGethAdmin. Also, fixed test due to typing

* made suggested changes

* made suggested changes

* fixed spelling errors

* added test and docs

* newsfragment

* merge conflict

* remove setSolc

* copy in suggested test

* forgot to check liniting before commit

* linting

* linting

* Properly initialize external modules

Properly initialize modules that do not inherit from the `web3.module.Module` class. We weren't properly testing self-referential, non-static methods with this functionality and so a test was also added for this.

* correct docs for external modules

* Refactor attach_module logic

* Allow for accepting the ``Web3`` instance as the first argument in any module's ``__init()`` method, rather than requiring a module to inherit from ``web3.module.Module`` if it needs to make use of the ``Web3`` instance.

* Update tests to test the above change.

* Add a more friendly error message if the module has more than one __init__() argument. Add test for this error message / case.

* recorrect docs for external modules

* Compile release notes

* Bump version: 5.27.0 → 5.28.0

* Add 'Breaking Changes' and 'Deprecation' to our valid newsfragment types (ethereum#2340)

* Add 'Breaking Change' and 'Deprecation' to our valid newsfragment types

* Add newsfragment for new newsfragment categories

* Remove removal section of release notes

* Drop python 3.6 (ethereum#2343)

* Drop python 3.6

* Remove parity tests

* Add newsfragment for py36 drop

* Fix gas types (ethereum#2330)

* fix: correct type for effectiveGasPrice (Wei, not int)

* fix: correct type for gas and gas_limit (int, not Wei)

* lint: removed unused type imports of Wei

* Add newsfragment for Wei/int typing fixes

Co-authored-by: kclowes <kclowes@users.noreply.github.com>

* Upgrade websockets dependency to v10+ (ethereum#2324)

* Require websockets v10+

- Remove event loop parameter

* Add newsfragment for websockets upgrade

* ➕ Add Python 3.10 support (ethereum#2175)

* ➕ Add Python 3.10 support to CI

* Dropped support for all parities

* Change docker image to use 3.10

* Update pytest-asyncio plugin

* Mark async fixture as such, clean up pytest DeprecationWarnings

Signed-off-by: Harmouch101 <mahmoudddharmouchhh@gmail.com>

Co-authored-by: Felipe Selmo <fselmo2@gmail.com>
Co-authored-by: kclowes <kclowes@users.noreply.github.com>

* add fork description

* [NBA-39] add multiple nodes for web3 HTTPProvider (#1)

* Update README.md

* Update README.md

* Update README.md

* typo fix

Co-authored-by: pacrob <paul@pacrob.com>
Co-authored-by: AlwaysData <dbfreem2@yahoo.com>
Co-authored-by: alex <67626131+alexpvpmindustry@users.noreply.github.com>
Co-authored-by: Felipe Selmo <fselmo2@gmail.com>
Co-authored-by: DefiDebauchery <75273961+DefiDebauchery@users.noreply.github.com>
Co-authored-by: Mikko Ohtamaa <mikko@opensourcehacker.com>
Co-authored-by: kclowes <kclowes@users.noreply.github.com>
Co-authored-by: Marek Šuppa <mrshu@users.noreply.github.com>
Co-authored-by: broper2 <brianroper7@gmail.com>
Co-authored-by: Călina Cenan <calina@cenan.net>
Co-authored-by: Harmouch101 <mahmoudddharmouchhh@gmail.com>
Co-authored-by: Marc Garreau <marcdgarreau@gmail.com>
Co-authored-by: coccoinomane <guido.pettinari@gmail.com>
@dbfreem dbfreem deleted the feature/async_geth_personal branch July 9, 2022 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pacrob pacrob pacrob left review comments

@kclowes kclowes kclowes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dbfreem @pacrob @kclowes