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

Change ABIFunctionInfo to ABIElementInfo #85

Merged
merged 3 commits into from
Jun 27, 2024

Conversation

reedsa
Copy link
Contributor

@reedsa reedsa commented Jun 25, 2024

What was wrong?

After using ABIFunctionInfo I realized it should actually encompass all ABI element types. This should be named ABIElementInfo to match the new get_element_info utility in web3.

More context:
Using this in ethereum/web3.py#3408

Here's the implementation of get_element_info
https://github.com/ethereum/web3.py/blob/23378b4c470230684cf8298fd6133238867153bc/web3/utils/abi.py#L187

Related to Issue #
Closes #

How was it fixed?

Replace ABIFunctionInfo with ABIElementInfo and allow all ABIElement types as values of the abi attribute.

Todo:

  • Clean up commit history

  • Add or update documentation related to these changes

  • Add entry to the release notes

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

reedsa added a commit to reedsa/eth-typing that referenced this pull request Jun 25, 2024
@reedsa reedsa marked this pull request as ready for review June 25, 2024 16:47
reedsa added a commit to reedsa/eth-typing that referenced this pull request Jun 25, 2024
@reedsa reedsa requested review from fselmo, pacrob and kclowes June 25, 2024 20:19
@reedsa
Copy link
Contributor Author

reedsa commented Jun 25, 2024

We may want to deprecate ABIFunctionInfo in v4, but that will require a separate release branch since I've merged in stuff for the beta release. Let me know if we should proceed with that @fselmo @kclowes @pacrob

Copy link
Contributor

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

Some minor comments. I like the idea of deprecating but I could also do without it since it existed for a very brief period of time in this code base and it's 99.9% likely that we were the only ones that ever made use of it.

That said I also think creating a new branch to issue that deprecation wouldn't take too much dev time so I could do things "the right way" too I suppose 😄. No strong opinion there.

eth_typing/abi.py Outdated Show resolved Hide resolved
eth_typing/abi.py Outdated Show resolved Hide resolved
eth_typing/abi.py Outdated Show resolved Hide resolved
eth_typing/abi.py Outdated Show resolved Hide resolved
newsfragments/85.feature.rst Outdated Show resolved Hide resolved
reedsa added a commit to reedsa/eth-typing that referenced this pull request Jun 25, 2024
Copy link
Contributor

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

lgtm !

@reedsa
Copy link
Contributor Author

reedsa commented Jun 25, 2024

Here's a v4 branch to capture the deprecation - https://github.com/ethereum/eth-typing/pull/86/files

Copy link
Contributor

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@reedsa reedsa merged commit a026336 into ethereum:main Jun 27, 2024
20 checks passed
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