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

Format 'from' and 'to' fields to checksum addresses in RECEIPT_FORMAT… #1562

Merged
merged 1 commit into from Jan 15, 2020
Merged

Format 'from' and 'to' fields to checksum addresses in RECEIPT_FORMAT… #1562

merged 1 commit into from Jan 15, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 3, 2020

What was wrong?

Receipt formatter was not formatting from and to addresses to checksum as it does with transaction formatter.

Related to Issue #1547

How was it fixed?

Followed @ignition42's instructions.

Added from and to here: https://github.com/ethereum/web3.py/blob/v5.4.0/web3/_utils/method_formatters.py#L174

As they were here: https://github.com/ethereum/web3.py/blob/v5.4.0/web3/_utils/method_formatters.py#L119

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

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 @rajeev29five! Do you mind adding an assertion to the integration tests? Maybe something like assert is_checksum_address(receipt['to']) in the test_eth_getTransactionReceipt_mined test in web3/_utils/module_testing/eth_module.py. Let me know if that's unclear or you need more direction!

@ghost
Copy link
Author

ghost commented Jan 8, 2020

Ok @kclowes I'll do that. If I'm stuck I'll ask for help.
I think I'll have to add something like ...receipt['from'] too. Right?

@kclowes
Copy link
Collaborator

kclowes commented Jan 8, 2020

That would be ideal, but you may need to also add a check to make sure it's there since we're doing the is_not_null check for the from formatter. I didn't check that 'from' comes back with the transaction receipts in those tests. Thank you!

@ghost
Copy link
Author

ghost commented Jan 10, 2020

@kclowes looks like from and to key do not come back with transaction receipt. Little guidance please.
Thank you!

@kclowes
Copy link
Collaborator

kclowes commented Jan 10, 2020

@rajeev29five let's add the xfail decorator (@pytest.mark.xfail(raises=KeyError)) to the test for the ethtester integration tests. So in tests/integration/test_ethereum_tester.py, make a new method in the TestEthereumTesterEthModule class:

@pytest.mark.xfail(raises=KeyError, reason="ethereum tester doesn't return 'to' key")    
def test_eth_getTransactionReceipt_unmined(self, eth_tester, web3, unlocked_account):
        super().test_eth_getTransactionReceipt_unmined(web3, unlocked_account)

@ghost
Copy link
Author

ghost commented Jan 10, 2020

@kclowes Need some help with these Failures

@@ -298,6 +298,9 @@ def test_eth_chainId(self, web3):
assert is_integer(chain_id)
assert chain_id == 61

@pytest.mark.xfail(raises=KeyError, reason="ethereum tester doesn't return 'to' key")
def test_eth_getTransactionReceipt_unmined(self, eth_tester, web3, unlocked_account):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this should be def test_eth_getTransactionReceipt_mined. Sorry I mistyped earlier.

@@ -758,6 +758,7 @@ def test_eth_getTransactionByBlockNumberAndIndex(
assert is_dict(transaction)
assert transaction['hash'] == HexBytes(mined_txn_hash)

@pytest.mark.xfail(raises=KeyError)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the higest level that gets run for each different client (ethtester, geth, parity). Since only ethtester was failing, you'll need to remove this here. It will be overridden for ethtester where you have it above in tests/integration/test_ethereum_tester.py. Since we don't expect a failure for geth or parity (I don't think), you can remove the xfail here.

@@ -298,6 +298,9 @@ def test_eth_chainId(self, web3):
assert is_integer(chain_id)
assert chain_id == 61

@pytest.mark.xfail(raises=KeyError, reason="ethereum tester doesn't return 'to' key")
def test_eth_getTransactionReceipt_mined(self, eth_tester, web3, unlocked_account):
super().test_eth_getTransactionReceipt_unmined(web3, unlocked_account)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rajeev29five this line should also be test_eth_getTransactionReceipt_mined. I think then you'll be in good shape as soon as you fix the lint tests.

Copy link
Author

@ghost ghost Jan 13, 2020

Choose a reason for hiding this comment

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

@kclowes I was about to ask you about that line but anyways I will make the changes and test for any other failures. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

@kclowes What should I pass for positional argument: 'mined_txn_hash' ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll need to pass in web3, block_with_txn, and mined_txn_hash, just like in the original definition in module_testing/eth_module.py. So the function should look like:

@pytest.mark.fail(...)
def test_eth_getTransactionReceipt_mined(self, web3, block_with_txn, and mined_txn_hash):
        super().test_eth_getTransactionReceipt_mined(web3, block_with_txn, and mined_txn_hash)

Sorry for all the confusion. Let me know if you want a better explanation of why.

@ghost
Copy link
Author

ghost commented Jan 14, 2020

All checks passed. Thanks for all the help : ) @kclowes

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.

Awesome 🎉! Thanks for your persistence, @rajeev29five!

…TERS

Add newsfragment for checksum address in txn receipt
@kclowes kclowes merged commit d9265d6 into ethereum:master Jan 15, 2020
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