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

Add API methods to ABIType for web3 #118

Merged
merged 1 commit into from
Feb 21, 2019
Merged

Conversation

davesque
Copy link
Contributor

What was wrong?

Web3.py still uses the deprecated process_type function which assumes all type strings are terminal types (non-tuple types).

How was it fixed?

This PR adds some API features to the AST classes whose instances are output by eth-abi's new parsing facilities. These new API features can then be utilized in web3.py to remove the old process_type function.

Cute Animal Picture

Cute animal picture

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.

This looks solid. I assume these are all internal APIs or is there a documentation update that needs to be made anywhere?

@davesque
Copy link
Contributor Author

@pipermerriam I suppose this part of eth-abi could maybe use a documentation update. Since we're going to pull it into web3.py, it's starting to look like public API but I don't think there's any section in the docs for it. I'd like to address that in a separate issue ticket if possible so it doesn't block the related web3 work for too long.

@pipermerriam
Copy link
Member

I'm fine with docs happening separately but they should happen asap so we don't forget.

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

LGTM. Pointing out one thing that maybe doesn't need to be public, which gives us more flexibility later if we want to refactor.

)
def test_abi_type_item_type(type_str):
abi_type = parse(type_str)
assert not abi_type.has_dynamic_arrlist
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test looks repeated. Based on the name, maybe it was supposed to test something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops!

raise NotImplementedError('Must implement `is_dynamic`')

@property
def has_dynamic_arrlist(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does has_dynamic_arrlist need to be public? Seems like it's mostly a helper property, so my instinct is to _-prefix it until we know we want to use it somewhere.

@davesque davesque merged commit 33fbe30 into ethereum:master Feb 21, 2019
@davesque davesque deleted the is-dynamic branch February 21, 2019 01:44
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.

3 participants