Tiffany/snake case

[tmckenzie51]

What was wrong?

Parity Personal methods were camelCased rather than snake_cased

Related to Issue #
1429

How was it fixed?

Parity Personal method names were changed from camelCase to snake_case. Deprecation warnings and tests for the snake_cased methods were also added.

Todo:

• Add entry to the release notes

Cute Animal Picture

[tmckenzie51]

Hi. In the example shared under issue #1429 for the geth admin module, there were changes to be made to integration tests. However, I was a bit confused about how I could modify similar tests for the parity personal module in the common.py, http.py, ipc.py, and ws.py files. I notice for the most part that the modified code in these integration test files was just functions with this line of code: super().test_admin_start_stop_rpc(web3). However, I think I am having trouble understanding how exactly this tests the module, and how to adjust that appropriately for the parity personal module. Also for the core tests, based on the geth admin PR example, it seems as though there were individual core test files such as 'test_admin_addPeer.py' that were modified for different geth admin methods. However, I don't see files like this for the parity personal module. Should I create these files? Would you be able to assist me with this? @kclowes @pipermerriam

[kclowes]

Thanks for getting this started @tmckenzie51!

I was a bit confused about how I could modify similar tests for the parity personal module in the common.py, http.py, ipc.py, and ws.py files.

If you follow the inheritance up from the individual _http, _ipc, and _ws files, eventually you will land in the GoEthereumPersonalModuleTest or the ParityPersonalModuleTest. We have the different connection files so that we can modify tests if something works differently over the IPC connection vs. HTTP, for example. web3/_utils/module_testing/personal_module.py is where most (I think all, but haven't looked to confirm) of the integration tests live for the parity personal module. So you should be able to add the _deprecated suffix to the tests that have the camel case methods, and then pretty much copy/paste the existing tests and swap out the camel case methods for the new snake case methods. Let me know if that doesn't make sense and I can write up something a little more complete!

[tmckenzie51]

@kclowes Thank you for your helpful feedback. I followed the step you described above in your comment and made the necessary changes to the personal_module.py file on both GoEthereumPersonalModuleTest and ParityPersonalModuleTest classes. What is the difference between these 2 classes? I notice that they both seem to have similar test methods despite being 2 separate classes. Is there a reason for this that could help me understand the codebase better?

Also, after making the changes mentioned in your comment above, I was able to make some progress. However, I have been getting CI errors related to one of the _deprecated tests. The link to an example of this CI test error is here - https://circleci.com/gh/ethereum/web3.py/100596?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link . For example, in this test, the error arises when test_personal_importRawKey_deprecated is called. This returns the error ‘account already exists’. I’m wondering if the new snake_cased tests are being run first, then the deprecated tests are being run after, which could potentially lead to an account state tracking error, which would trigger that error message. Would you be able to comment on this?

[kclowes]

I notice that they both seem to have similar test methods despite being 2 separate classes.

Yep, they do. We split them up because the Geth and Parity clients sometimes have different implementations of the same methods. Instead of having if statements in our code to check which client a web3 instance is using, the API is instead web3.geth. ... or web3.parity. ..., and if they're different we can change the expectations in the tests. We anticipate them diverging more and more as time goes on.

This returns the error ‘account already exists’. I’m wondering if the new snake_cased tests are being run first, then the deprecated tests are being run after, which could potentially lead to an account state tracking error, which would trigger that error message.

Yep, I think you're absolutely right. I would just add a new key by changing a few digits in the old key, and then use the new address it comes up with. Let me know if that doesn't make sense!

[tmckenzie51]

Yep, they do. We split them up because the Geth and Parity clients sometimes have different implementations of the same methods. Instead of having if statements in our code to check which client a web3 instance is using, the API is instead web3.geth. ... or web3.parity. ..., and if they're different we can change the expectations in the tests. We anticipate them diverging more and more as time goes on.

Oh okay. That makes sense. Thank you for explaining that.

Yep, I think you're absolutely right. I would just add a new key by changing a few digits in the old key, and then use the new address it comes up with. Let me know if that doesn't make sense!

Oh okay. When I change a few digits of the hexadecimal private key (NEW_PRIVATE_KEY_HEX), and then call importRawKey with the NEW_PRIVATE_KEY_HEX and PASSWORD parameters, it will return the new_actual address. In the assertion, I would then have new_actual == NEW_ADDRESS. However, since the ADDRESS previously used was a hardcoded global variable, what would be the NEW_ADDRESS for this assertion? I’m assuming that this value has to be obtained outside of using the same function that we’re trying to test. How can I obtain the NEW_ADDRESS value without using importRawKey, in order to hard code it and test if the importRawKey method is working correctly?

[kclowes]

How can I obtain the NEW_ADDRESS value without using importRawKey, in order to hard code it and test if the importRawKey method is working correctly?

I think just using the address that it comes up with is sufficient. Then we'll know if we make a breaking change in the future. But, if you really wanted to be thorough, you could use eth_account to create an address and then make sure that importRawKey returns the same one. So it might look something like:

from eth_account import Account
acct = Account.create('some-password')
private_key = acct.key
raw_key_addr = w3.geth.personal.importRawKey(acct.key, 'some-password')
assert acct.address == raw_key_addr

You may need to do some tweaking to the above code, but that's the general gist. Let me know if you have other questions!

[tmckenzie51]

@kclowes Thank you for your help with that bug! For the importRawKey deprecated tests, I did pytest.raises(ValueError) since I would always expect test_importRawkey_deprecated to always return the 'account already exists' error. That led to most of the CI tests passing, at least the ones that were having errors triggered by this test.

However, there's an error that I did not previously notice, which is related to both test_personal_unlock_account_failure and test_personal_unlockAccount_failure_deprecated . For both tests, the CI test fails with the error message "DID NOT RAISE <class 'ValueError'>". See: https://circleci.com/gh/ethereum/web3.py/101784?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link . I'm not sure why this would happen. I made very few changes to these methods - mostly just putting their names to snake_case. Do you have any idea as to what may be causing these errors?

[kclowes]

Thanks @tmckenzie51!