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

signer/fourbyte: add support for nested types in selectors #24407

Merged
merged 1 commit into from Mar 4, 2022

Conversation

danhper
Copy link
Contributor

@danhper danhper commented Feb 16, 2022

The selector parsing logic in fourbytes has been improved to work with tuples.
This requires changing the current regexp-based approach to a small hand-written parser.
Among potentially other things, this allows to call functions that have tuples in their signatures from clef, which would previously fail with the following error: ValueError: validation failed: Transaction contains data, but the ABI signature could not be found: signature 40b1eb10 not found

@danhper danhper requested a review from holiman as a code owner Feb 16, 2022
@fjl
Copy link
Contributor

fjl commented Feb 16, 2022

I feel like there might be some overlap with package accounts/abi here. I think we should try to keep ABI-related parsing in that package, and then use it in signer/fourbyte.

@fjl
Copy link
Contributor

fjl commented Feb 16, 2022

Please check if accounts/abi has suitable functionality, and if not, move the new parsing functions there.

@danhper
Copy link
Contributor Author

danhper commented Feb 22, 2022

Hi @fjl, thanks for the feedback!
I don't believe there was such functionality yet, so I moved it to the abi package.
Cheers!

@danhper
Copy link
Contributor Author

danhper commented Mar 4, 2022

Hi @fjl , please let me know if there's any other change you'd like me to make on this.
Thanks a lot!

@fjl fjl changed the title Allow to use nested types in fourbyte signer/fourbyte: add support for nested types Mar 4, 2022
@fjl fjl changed the title signer/fourbyte: add support for nested types signer/fourbyte: add support for nested types in selectors Mar 4, 2022
@fjl fjl merged commit 37f9d25 into ethereum:master Mar 4, 2022
1 check passed
@fjl fjl added this to the 1.10.17 milestone Mar 4, 2022
@fjl
Copy link
Contributor

fjl commented Mar 4, 2022

Thanks for putting in the work to make this change in a clean way!

@danhper danhper deleted the fix-fourbyte-signatures branch Mar 7, 2022
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Mar 8, 2022
…24407)

This replaces the simple selector parser in signer/fourbyte with one that
can actually handle most types. The new parser is added in accounts/abi
to also make it useable elsewhere.
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
…24407)

This replaces the simple selector parser in signer/fourbyte with one that
can actually handle most types. The new parser is added in accounts/abi
to also make it useable elsewhere.
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.

None yet

2 participants