Skip to content

Conversation

@kclowes
Copy link
Collaborator

@kclowes kclowes commented Oct 24, 2019

What was wrong?

There were a couple duplicate functions in both eth-utils and web3. We want to standardize on eth-utils.

Related to Issue #1197

How was it fixed?

Took out the duplicate methods from web3, and added a note to the v6 breaking changes for apply_formatters_to_array which is very close but not quite the same between the two libraries.

Todo:

Cute Animal Picture

image

@kclowes kclowes changed the title [WIP] Remove most duplicate methods from eth-utils [WIP] Use eth-utils methods instead of custom web3 methods Oct 24, 2019
@kclowes kclowes changed the title [WIP] Use eth-utils methods instead of custom web3 methods Use eth-utils methods instead of custom web3 methods Oct 24, 2019
@kclowes kclowes requested a review from pipermerriam October 24, 2019 22:54
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.

Since apply_formatters_to_array is under the web3._utils namespace we don't have to maintain backwards compatibility since it's considered a private API (so no need to wait till v6 nor is there need to make the existing formatters importable from web3._utils.formatters

@kclowes
Copy link
Collaborator Author

kclowes commented Oct 25, 2019

@pipermerriam I think it is a breaking change because we use it in middleware? The web3 apply_formatters_to_array converts the result to a list, but the eth-utils version does not. Because we use the web3 apply_formatters_to_array in the pythonic formatting middleware, a bunch of the ENS tests started failing because they were expecting a list to be returned from w3.eth.accounts, but got a tuple instead. I suspect others may run into the same problem. Maybe it's better to use the eth-utils apply_formatters_to_array and convert to a list on the web3 side?

@pipermerriam
Copy link
Member

Maybe it's better to use the eth-utils apply_formatters_to_array and convert to a list on the web3 side?

That is what I would advocate for (and ideally leave a note to do any cleanup in v6 if you think it's worth removing the shim then)

@kclowes kclowes merged commit e086cf2 into ethereum:master Nov 4, 2019
@kclowes kclowes deleted the use-eth-utils branch November 4, 2019 17:23
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