search

ethereum / eth-abi

Ethereum ABI utilities for python
MIT License
241 stars 268 forks source link

Remove `process_type` utility function in favor of `grammar.parse` #85

Closed davesque closed 6 years ago

davesque commented 6 years ago

What was wrong?

The process_type utility function (found here) is left over from the previous style of parsing type strings. The parse function in the grammar module (found here) provides the same functionality and is more extensible.

How can it be fixed?

We should remove the process_type function in favor of the grammar.parse function.

carver commented 6 years ago

It's used a few places in web3, so that's one place to try out the new approach.

davesque commented 6 years ago

@carver My thinking was that this could be something that gets included in v2.

pipermerriam commented 6 years ago

If web3 is using this we need to remove that since this API is under utils and thus, is considered private. I don't think this requires a deprecation, but we should be careful to not break web3 as we fix this.

davesque commented 6 years ago

@pipermerriam @carver

Another behavior that I'd like to change, which is related to all of this, is that the encode_single and decode_single functions accept either a type string or a type tuple (with "base", "sub", and "arrlist" components). If a type tuple is provided, the collapse_type utility function is used to convert it to a type string.

I'd like to remove the collapse_type function and make it so that the encode_single and decode_single functions expect a type string and nothing else. My motivation with this is to make the API simpler and less magical.

I feel like I might have had a previous discussion about this with @carver in which he may have asked to leave this behavior unchanged. Sorry if I'm forgetting this. If that is the case, would it be possible to simply move the collapse_type function into web3 or wherever it's needed and apply it manually to type string data that is headed for encode_single or decode_single?

davesque commented 6 years ago

^^^ Actually, I'll make another issue ticket for the last comment.

davesque commented 6 years ago

I went ahead and made PR #87 for when we decide to include this.

dylanjw commented 6 years ago

I can open a PR in web3 to replace process_type with grammar.parse

davesque commented 6 years ago

Merged PR #87 .