Permitted bytes addresses in web3 #893

voith · 2018-06-09T20:01:11Z

What was wrong?

Web3 did not permit bytes addresses.
see #884

How was it fixed?

Permitted bytes addresses in web3.

TODO LIST

Add tests
update docs

Cute Animal Picture

voith · 2018-06-11T07:15:43Z

@pipermerriam @carver I have added some tests to check that my code is functionally complete.
The integration tests, test some parts of contracts and filters. However, I might need more tests there.
Let me know if I'm missing any tests or if you'd like me to add more tests.
Currently, I have modified the existing tests to accept both binary and hex addresses. I'm not sure if this is the right approach. I wanted to verify that the code works, so to get it done fast I modified the existing tests. If you guys want this tested in a completely separate module then I'm fine with that.

For the ease of reviewing I have separated the code change from the tests by splitting them into two separate commits.

Also, I had some trouble with nonce. w3.eth.getTransactionCount doesn't seem to increment the count, once a transaction has been sent. I tried using waitForTransactionReceipt but that didn't change anything.

web3.py/web3/utils/module_testing/eth_module.py

Line 223 in 70b7ded

'nonce': web3.eth.getTransactionCount(unlocked_account),

I was getting this error with geth:

ValueError: {'code': -32000, 'message': 'known transaction: 157d2598aa226207eb4711e04511266f0ad50817d6654c0a0835ae2b3dd3fa7e'}

In such cases I left the tests as is.

PS: I'll update docs once I get some feedback on this!

carver

Approach looks good 👍

carver · 2018-06-11T16:57:49Z

tests/core/contracts/test_contract_init.py

+
+
+@pytest.fixture(params=['hex', 'bytes'])
+def math_addr(request, math_deploy_receipt):


Yup, the approach of parametrizing the address seems totally reasonable 👍

carver · 2018-06-11T17:02:28Z

tests/core/contracts/test_contract_init.py

+def math_addr(request, math_deploy_receipt):
+    addr = math_deploy_receipt['contractAddress']
+    if request.param == 'bytes':
+        return to_bytes(hexstr=addr)


I considered recommending eth_utils.to_canonical_address(addr) here, but I find it hard to remember that canonical means the bytes version (to me the semantically "canonical" version of the address is the checksum version).

So I actually like to_bytes for clarity here.

carver · 2018-06-11T17:03:33Z

tests/integration/test_ethereum_tester.py

+@pytest.fixture(scope="module", params=['bytes', 'hex'])
+def math_contract_address(math_contract, request):
+    if request.param == 'bytes':
+        return math_contract.address


conversion missing?

Oops, Nice catch!!!

carver · 2018-06-11T17:25:23Z

tests/integration/test_ethereum_tester.py

+
+
+@pytest.fixture
+def unlocked_account_dual_type(web3, unlockable_account_dual_type, unlockable_account_pw):


I liked the other approach in this PR: replace the existing fixture with a parametrized one, as opposed to leaving the old one and adding a new one with a different name.

I had to do this because some tests were failing with the parametrized approach. Its the same issue that I've mentioned in #893 (comment).

carver · 2018-06-11T17:27:46Z

web3/utils/rpc_abi.py

@@ -24,6 +24,7 @@

 FILTER_PARAMS_ABIS = {
    'to': 'address',
+    'address': 'address'


nitpick add trailing comma

carver · 2018-06-11T17:28:06Z

web3/utils/rpc_abi.py

@@ -47,6 +48,9 @@
    'eth_sign': ['address', 'bytes'],
    # personal
    'personal_sendTransaction': TRANSACTION_PARAMS_ABIS,
+    'personal_lockAccount': ['address'],
+    'personal_unlockAccount': ['address', None, None],
+    'personal_sign': [None, 'address', None]


nitpick add trailing comma

carver · 2018-06-11T17:34:20Z

w3.eth.getTransactionCount doesn't seem to increment the count, once a transaction has been sent

That doesn't sound right. Something else must be going on. Can you leave in an example of a broken test with getTransactionCount?

I was getting this error with geth

Right, I think the integration tests don't reset state in between tests.

In such cases I left the tests as is.

That should be fine, although it should be possible to parametrize non-state-changing tests with both kinds of addresses, like a getBalance(address) test.

voith · 2018-06-11T19:11:55Z

Thanks @carver for the review!

That doesn't sound right. Something else must be going on. Can you leave in an example of a broken test with getTransactionCount?

Quite possible that something else is going wrong. I did not spend any time digging it. But simply using paramaterized unlocked_account caused the test failures for me. I'll try to come up with a test case to replicate the issue tomorrow.

Besides do you have any ideas what tests I might be missing? Or any other tests that you would like to see added. Although this code won't break anything, I want to make sure that I've covered most cases where binary addresses will be used.

carver · 2018-06-11T23:35:18Z

Besides do you have any ideas what tests I might be missing? Or any other tests that you would like to see added.

Maybe search through this API list for account? The first one I found that I'm not sure is covered is getStorageAt.

Maybe I missed it, but tests for addresses being passed in as arguments to contract function calls.

Checking that w3.eth.account.signTransaction accepts transactions with from & to addresses in bytes.

Also, the ens module.

Filters: contract address, or indexed address arguments.

voith · 2018-06-18T13:15:08Z

@carver I tried to figure out what was going wrong with test_eth_sendTransaction_with_nonce. This time I tested with parity. Like I said before web3.eth.getTransactionCount(unlockable_account_dual_type) returns the same value every time.
On inspection, web3.eth.waitForTransactionReceipt(txn_hash) returned the following

AttributeDict({'blockHash': None, 'blockNumber': None, 'contractAddress': None, 'cumulativeGasUsed': 21000, 'gasUsed': 21000, 'logs': [], 'logsBloom': HexBytes('0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000'), 'root': '0x979928a2ecf81da90713b24af9b83cbc4ad59e7e7096ce86ff62ad1780c1d677', 'status': None, 'transactionHash': HexBytes('0x66a9cc4cf67fadda3d0691756d8c355b9ad77beb71ca1311467b6d8ad580dd01'), 'transactionIndex': 0})

As you can see blockNumber is None in this case. My guess is that the private chain instantiated doesn't mine(Not very sure though). @dylanjw Can you validate this statement?(since you wrote the parity tests).

Also, I got the test passing by doing the following:

count = 0


def _counter():
    global count
    while True:
        yield count
        count += 1

counter = _counter()

class EthModuleTest:
   def test_eth_sendTransaction_with_nonce(self, web3, unlockable_account_dual_type):
        txn_params = {
          ...
         'nonce': web3.eth.getTransactionCount(unlockable_account_dual_type) + next(counter),
        }

which clearly shows that the state is not being updated! I'll leave such tests as is and focus on the missing tests to get this PR completed.

voith · 2018-06-18T15:06:48Z

carver · 2018-06-18T21:46:36Z

My guess is that the private chain instantiated doesn't mine

Yeah, that sounds right to me. I don't think we're using the geth --dev equivalent for parity on the integration tests. (Actually, I don't think we're using geth --dev on the geth tests either)

carver · 2018-06-22T23:29:01Z

tests/core/contracts/conftest.py

@@ -15,6 +20,11 @@
 CONTRACT_ABI = json.loads('[{"constant":false,"inputs":[],"name":"return13","outputs":[{"name":"result","type":"int256"}],"type":"function"},{"constant":true,"inputs":[],"name":"counter","outputs":[{"name":"","type":"uint256"}],"type":"function"},{"constant":false,"inputs":[{"name":"amt","type":"uint256"}],"name":"increment","outputs":[{"name":"result","type":"uint256"}],"type":"function"},{"constant":false,"inputs":[{"name":"a","type":"int256"},{"name":"b","type":"int256"}],"name":"add","outputs":[{"name":"result","type":"int256"}],"type":"function"},{"constant":false,"inputs":[],"name":"increment","outputs":[{"name":"","type":"uint256"}],"type":"function"},{"constant":false,"inputs":[{"name":"a","type":"int256"}],"name":"multiply7","outputs":[{"name":"result","type":"int256"}],"type":"function"},{"anonymous":false,"inputs":[{"indexed":false,"name":"value","type":"uint256"}],"name":"Increased","type":"event"}]')  # noqa: E501


+@pytest.fixture(params=[lambda x: to_bytes(hexstr=x), identity])
+def address_conversion_func(request):


Cool, this approach seems like a clean way to test the different accepted address formats. 👍

Instead of repeating it in several conftests, it seems like a reasonable thing to put it in the root tests/conftest.py.

voith · 2018-06-24T13:20:35Z

@carver Ready for review!

added trailing commas

added missing conversion

changed fixture scope to module

voith · 2018-07-04T06:25:13Z

Thanks, @carver and @pipermerriam.

This isn't necessary since ethereum/web3.py#893

voith mentioned this pull request Jun 9, 2018

Permit bytes addresses for programmatic API usage #884

Closed

voith force-pushed the permit-binary-addresses branch from a20ddf2 to 9d49eee Compare June 11, 2018 06:59

voith force-pushed the permit-binary-addresses branch 2 times, most recently from 0aad54c to efddf0e Compare June 11, 2018 07:40

carver reviewed Jun 11, 2018

View reviewed changes

voith force-pushed the permit-binary-addresses branch from efddf0e to 920bc41 Compare June 18, 2018 07:11

voith force-pushed the permit-binary-addresses branch from ec15217 to d6d4fe2 Compare June 22, 2018 08:54

carver reviewed Jun 22, 2018

View reviewed changes

voith changed the title ~~[WIP]Permitted bytes addresses in web3~~ Permitted bytes addresses in web3 Jun 24, 2018

voith force-pushed the permit-binary-addresses branch from 90960d3 to 4d46cf5 Compare June 24, 2018 13:29

voith mentioned this pull request Jun 25, 2018

Parity (sometimes) returns transaction receipts before they are mined #936

Closed

voith and others added 11 commits June 29, 2018 11:25

Permitted byte addresses in web3

c7b2700

added trailing commas

added tests for permitting binary addresses in web3

4256936

added missing conversion

Docs: mentioned that web3 now permits 20 byte binary addresses

c439465

added more tests to check eth module compatability with binary addresses

5961380

test compatibility of filters with binary addresses

cf3e587

test contracts with binary addresses

2e3b8af

added conversion params for trace_call to work with binary addresses

5cd168f

added tests for ens to permit binary addresses

2d27c05

moved address_conversion_func fixture to tests/conftest.py

b1d269d

changed fixture scope to module

typo: added missing params

4c3f6b2

Document address types in preferential order

43fac7f

carver force-pushed the permit-binary-addresses branch from 5badde8 to 43fac7f Compare June 29, 2018 18:25

carver added 3 commits June 29, 2018 13:00

xfail on ethpm tests & special import warning

7a0fb00

isort fixups

33c1f80

Parity updated checksum from md5 to sha256

54a1f19

carver referenced this pull request Jun 29, 2018

Remove the missing eth-pm package from stable web3

73bdb8d

voith mentioned this pull request Jul 3, 2018

Contract call optimization - remove round trip #944

Merged

pipermerriam approved these changes Jul 3, 2018

View reviewed changes

pipermerriam merged commit 9800597 into ethereum:master Jul 3, 2018

voith deleted the permit-binary-addresses branch July 4, 2018 02:00

pipermerriam mentioned this pull request Jul 13, 2018

Implement Deployments.get_contract_instance ethpm/py-ethpm#73

Merged

palango mentioned this pull request Jan 20, 2020

Don't encode addresses as checksums with web3 raiden-network/raiden#5700

Merged

palango added a commit to raiden-network/raiden that referenced this pull request Jan 20, 2020

Don't encode addresses as checksums with web3

4e60b23

This isn't necessary since ethereum/web3.py#893

hackaugusto pushed a commit to raiden-network/raiden that referenced this pull request May 29, 2020

Don't encode addresses as checksums with web3

0863127

This isn't necessary since ethereum/web3.py#893

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permitted bytes addresses in web3 #893

Permitted bytes addresses in web3 #893

voith commented Jun 9, 2018 •

edited

Loading

voith commented Jun 11, 2018 •

edited

Loading

carver left a comment

carver Jun 11, 2018

carver Jun 11, 2018

carver Jun 11, 2018

voith Jun 11, 2018

carver Jun 11, 2018

voith Jun 11, 2018

carver Jun 11, 2018

carver Jun 11, 2018

carver commented Jun 11, 2018

voith commented Jun 11, 2018

carver commented Jun 11, 2018

voith commented Jun 18, 2018

voith commented Jun 18, 2018 •

edited

Loading

carver commented Jun 18, 2018

carver Jun 22, 2018

voith commented Jun 24, 2018

voith commented Jul 4, 2018



		@pytest.fixture(params=['hex', 'bytes'])
		def math_addr(request, math_deploy_receipt):



		@pytest.fixture
		def unlocked_account_dual_type(web3, unlockable_account_dual_type, unlockable_account_pw):

Permitted bytes addresses in web3 #893

Permitted bytes addresses in web3 #893

Conversation

voith commented Jun 9, 2018 • edited Loading

What was wrong?

How was it fixed?

TODO LIST

Cute Animal Picture

voith commented Jun 11, 2018 • edited Loading

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver commented Jun 11, 2018

voith commented Jun 11, 2018

carver commented Jun 11, 2018

voith commented Jun 18, 2018

voith commented Jun 18, 2018 • edited Loading

carver commented Jun 18, 2018

Choose a reason for hiding this comment

voith commented Jun 24, 2018

voith commented Jul 4, 2018

voith commented Jun 9, 2018 •

edited

Loading

voith commented Jun 11, 2018 •

edited

Loading

voith commented Jun 18, 2018 •

edited

Loading