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 get_abi_output_names utility method #2496

Closed
wants to merge 1 commit into from

Conversation

feulf
Copy link
Contributor

@feulf feulf commented Jun 1, 2022

What was wrong?

Very simply, there isn't a utility method to get the output names from a call, and we needed that.

Cute Animal Picture

image

@feulf feulf marked this pull request as ready for review June 1, 2022 14:25
@fselmo
Copy link
Collaborator

fselmo commented Jun 8, 2022

Hey @feulf 👋 ... thanks for the PR! I'd be happy to have this added. One not-so-ideal pattern I'm seeing is that the internal utility methods found at web3._utils seem to be useful for users even though they aren't technically meant to be made public. What that means is they could technically break across minor / patch versions.

In practice, I don't see that happening too often since they perform a particular useful functionality that is core to how the library works but things like method signatures, typing, etc... could technically "break" these internal utility methods and anyone using these methods would then have breaking changes across minor / patch versions.

I'm thinking since this new method isn't even used internally, adding them to the newly created web3.utils module for public API utility methods, would be best. If the input types are also useful to you, perhaps that could be entirely moved over there as well. We should also add documentation for it in the appropriate area for the utils module in the docs.

I'm thinking we can move all of that logic over to the external API module (web3.utils.abi, perhaps) and if any of the internal logic ever needs to deviate from these public utility methods, it can be created at that time to replace the public one, internally.

Thoughts?

cc: @kclowes

@kclowes
Copy link
Collaborator

kclowes commented Jun 8, 2022

@fselmo great ideas! I think a lot of the abi utility functions that we have do get used fairly frequently even though they are in _utils so it would be nice to expose a public utils folder 👍

@fselmo fselmo mentioned this pull request Aug 5, 2022
1 task
@fselmo
Copy link
Collaborator

fselmo commented Aug 5, 2022

closed in favor of #2596

@fselmo fselmo closed this Aug 5, 2022
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