Moved account to BaseEth

[dbfreem]

What was wrong?

Make account accessible in AsyncEth

closes #2580

How was it fixed?

Move Eth.account to BaseEth so that it can be used in AsyncEth.

Todo:

• Add entry to the release notes

[kclowes]

Thanks @dbfreem! Before this gets merged, we'll need to add a few tests. If you want to/have time to add them, that would be great! Otherwise someone from the team can pick it up!

You should be able to pretty much wholesale copy/paste tests/core/eth-module/test_async_accounts.py, but where you have your async_w3 fixture defined, you'll need to pass in the AsyncEth module. So instead of this line it should look something like:

return Web3(AsyncEthereumTesterProvider(), modules={"eth", (AsyncEth,)})

[dbfreem]

@kclowes So, I did some thinking about the test here. Two things I thought of.

1. Most of these test are testing the functionality of eth_account.Account. It could be argued that these test don't really belong in web3.py but actually in eth_account. They are actually probably already covered there.
2. If we do need test for this, shouldn't the test be only on BaseEth instead of duplicating them between AsyncEth and Eth? I could change the above referenced test to be only one set of test against BaseEth.account.

Just my thoughts so feel free to say "yes, we want both the AsynEth and Eth test in web3.py for account". :) haha.

[kclowes]

Thanks for the comments!

1. Most of these test are testing the functionality of eth_account.Account. It could be argued that these test don't really belong in web3.py but actually in eth_account. They are actually probably already covered there.

They are covered over there as well, but I think it's good to make sure we can access the most important methods through w3.eth.account, rather than just on the Account class. For example, when the HDWallet feature was implemented, it was working great in eth-account, but we had to do some tweaking to get it to work in web3.

2. If we do need test for this, shouldn't the test be only on BaseEth instead of duplicating them between AsyncEth and Eth? I could change the above referenced test to be only one set of test against BaseEth.account.

That's a good idea. I think it would be good to have at least one "happy path" test for each Eth and AsyncEth to make sure we can access account through each of those classes to prevent any regressions though.

[dbfreem]

@kclowes I made the suggested changes, take a look and let me know what you think

[fselmo]

I added a comment just to make sure we have decent assumptions for the testing around this. It would be good to add this feature to the async docs too but I can do that before merging if you'd like. Just let me know.

Thanks for the PR 👌

[dbfreem]

@fselmo I made the suggested changes. Copied your test into test_account.py, and made the doc changes.

[fselmo]

@fselmo I made the suggested changes. Copied your test into test_account.py, and made the doc changes.

lgtm! I just organized async at the bottom so it's a bit easier to read. Ty 👍🏼... I'll merge as soon as tests pass