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

Flag for Stricter ABI Type Checking #1419

Merged
merged 2 commits into from
Sep 20, 2019
Merged

Conversation

kclowes
Copy link
Collaborator

@kclowes kclowes commented Aug 13, 2019

What was wrong?

web3 v5 will accept a bytestring or a hexstring if a bytes value is specified in an ABI. Because an empty string can be decoded as hex, it wasn't raising a ValidationError like v4 did. It was also requested that we not allow bytes values that are shorter than the specified size.

Related to issue #1412

How was it fixed

This PR adds a stricter type checking mechanism for bytes values that is available on the web3 instance. Using w3.enable_strict_bytes_type_checking() will disallow strings that don't have a '0x', and will disallow byte strings that are different from the size the ABI specifies.

I also noticed that we were using eth-abi's codec for events instead of the one registered in web3, so changed that over to use the codec on the web3 instance.

  • Added a deprecation warning for strings without a '0x' - in v6 it will be invalid to pass a string without 0x to an ABI with bytes specified

Cute animal picture

Screen Shot 2019-08-19 at 5 03 16 PM

Todos:

  • don't allow strings without a '0x' prefix
  • don't allow bytestrings that are the wrong size
  • cleanup
  • docs
  • events
  • Fix decoder

@kclowes kclowes force-pushed the empty-str-flag branch 4 times, most recently from 602db5f to 9096240 Compare August 14, 2019 21:12
@kclowes kclowes force-pushed the empty-str-flag branch 2 times, most recently from 47089aa to 4647b7d Compare August 16, 2019 02:34
docs/overview.rst Outdated Show resolved Hide resolved
docs/overview.rst Outdated Show resolved Hide resolved
web3/_utils/abi.py Outdated Show resolved Hide resolved
web3/contract.py Show resolved Hide resolved
web3/main.py Outdated Show resolved Hide resolved
@kclowes kclowes changed the title [WIP] Flag for Stricter ABI Type Checking Flag for Stricter ABI Type Checking Aug 19, 2019
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Nice work.

conftest.py Show resolved Hide resolved
docs/contracts.rst Outdated Show resolved Hide resolved
web3/_utils/abi.py Outdated Show resolved Hide resolved
docs/contracts.rst Outdated Show resolved Hide resolved
tests/core/contracts/test_extracting_event_data.py Outdated Show resolved Hide resolved
tests/core/utilities/test_abi_is_encodable.py Outdated Show resolved Hide resolved
web3/_utils/abi.py Outdated Show resolved Hide resolved
web3/_utils/abi.py Outdated Show resolved Hide resolved
@kclowes kclowes changed the title Flag for Stricter ABI Type Checking [WIP]Flag for Stricter ABI Type Checking Aug 30, 2019
web3/_utils/abi.py Outdated Show resolved Hide resolved
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

some thoughts doing a quick look over this. Ping me for a real review whenever you're ready.

docs/contracts.rst Outdated Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
@kclowes kclowes changed the title [WIP]Flag for Stricter ABI Type Checking Flag for Stricter ABI Type Checking Sep 18, 2019
@kclowes
Copy link
Collaborator Author

kclowes commented Sep 18, 2019

@pipermerriam this is ready for another (and hopefully the last) round of real review

conftest.py Outdated Show resolved Hide resolved
docs/abi_types.rst Show resolved Hide resolved
docs/contracts.rst Outdated Show resolved Hide resolved
docs/contracts.rst Outdated Show resolved Hide resolved
tests/core/contracts/conftest.py Outdated Show resolved Hide resolved
tests/core/contracts/test_extracting_event_data.py Outdated Show resolved Hide resolved
tests/core/filtering/test_utils_functions.py Show resolved Hide resolved
tests/core/utilities/test_abi_is_encodable.py Outdated Show resolved Hide resolved
tests/core/utilities/test_abi_is_encodable.py Outdated Show resolved Hide resolved
web3/_utils/abi.py Show resolved Hide resolved
@kclowes kclowes merged commit 6d323e4 into ethereum:master Sep 20, 2019
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