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

Remove utf-8 string decoding from normalizers #974

Merged
merged 3 commits into from Aug 15, 2018

Conversation

@dylanjw
Copy link
Contributor

@dylanjw dylanjw commented Jul 30, 2018

What was wrong?

The eth-abi v2 alpha release adds utf-8 string decoding to the string type decoder. String decoding should therefore be removed from web3 return data normalizers.

Cute Animal Picture

image

Copy link
Member

@pipermerriam pipermerriam left a comment

I'm a litlte uncomfortable with this change. I think we should instead support the 1x and 2x line concurrently before dropping 1x support entirely. I'm up for being convinced otherwise and I understand that this adds some testing overhead.

My concern is that eth-abi>=2 has a breaking change and anyone who relies on both the web3 library and any low level functionality in the eth-abi library will be forced to abruptly upgrade their code without us having issued any deprecation warnings.

@dylanjw dylanjw force-pushed the dylanjw:eth_abi_string_decode branch 3 times, most recently from 59878f8 to a2533a7 Jul 30, 2018
@dylanjw dylanjw force-pushed the dylanjw:eth_abi_string_decode branch from a2533a7 to 2041b5d Jul 30, 2018
@dylanjw
Copy link
Contributor Author

@dylanjw dylanjw commented Jul 30, 2018

@pipermerriam Updated to add support for eth-abi >=2 without removing support for <2.

@@ -140,11 +144,14 @@ def abi_ens_resolver(w3, abi_type, val):


BASE_RETURN_NORMALIZERS = [
addresses_checksummed,
decode_abi_strings,
addresses_checksummed

This comment has been minimized.

@carver

carver Jul 30, 2018
Collaborator

style nitpick leave the trailing comma.

@@ -184,6 +188,9 @@ def test_call_get_string_value(string_contract, call):
assert result == "Caqalai"


@pytest.mark.skipif(
LooseVersion(eth_abi.__version__) >= LooseVersion("2"),
reason="eth-abi >=2 does utf-8 string decoding")
def test_call_read_string_variable(string_contract, call):
result = call(contract=string_contract,
contract_function='constValue')

This comment has been minimized.

@carver

carver Jul 30, 2018
Collaborator

Maybe add a second test that shows what happens on an ABI decoding error. We should probably do something like capture the eth-abi error and re-raise a generic ValueError.

This comment has been minimized.

@dylanjw

dylanjw Jul 30, 2018
Author Contributor

It raises a 'BadFunctionCallOutput' error

@dylanjw
Copy link
Contributor Author

@dylanjw dylanjw commented Aug 2, 2018

@pipermerriam @carver Ready for another review

setup.py Outdated
@@ -19,7 +19,7 @@
install_requires=[
"toolz>=0.9.0,<1.0.0;implementation_name=='pypy'",
"cytoolz>=0.9.0,<1.0.0;implementation_name=='cpython'",
"eth-abi>=1.1.1,<2",
"eth-abi==2.0.0a1",

This comment has been minimized.

@carver

carver Aug 2, 2018
Collaborator

We shouldn't require people to upgrade. We especially shouldn't require people to install an alpha dependency in a stable release. eth-abi>=1.1.1,<3 seems like a reasonable choice

@dylanjw dylanjw force-pushed the dylanjw:eth_abi_string_decode branch from 9fb151a to 2cc941e Aug 2, 2018
@dylanjw
Copy link
Contributor Author

@dylanjw dylanjw commented Aug 14, 2018

@pipermerriam @carver Is this good to merge?

@carver
carver approved these changes Aug 15, 2018
Copy link
Collaborator

@carver carver left a comment

Yup, good to go!

@dylanjw dylanjw merged commit fb30e87 into ethereum:master Aug 15, 2018
28 checks passed
28 checks passed
ci/circleci: doctest Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: py35-core Your tests passed on CircleCI!
Details
ci/circleci: py35-ens Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-ethtester-pyethereum Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-ethtester-pyevm Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-http-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-http-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-ipc-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-ipc-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-ws-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-ws-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-parity-http Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-parity-ipc Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-parity-ws Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py36-ens Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-ethtester-pyethereum Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-ethtester-pyevm Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-http-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-http-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-ipc-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-ipc-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-ws-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-ws-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-parity-http Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-parity-ipc Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-parity-ws Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants