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

Investigate and resolve discrepancies with solidity's abi.encode() #158

Merged

Conversation

fselmo
Copy link
Contributor

@fselmo fselmo commented Feb 10, 2022

What was wrong?

Related issues:

How was it fixed?

It seems that, for encode_abi(), extra 32-byte zero padding was happening in a few places, compared to what Solidity's abi.encode() returns. This seems to be the case when dealing with empty dynamic types, including empty dynamic arrays.

i.e.

  • encode_abi(['string'], ['']) vs Solidity abi.encode('')
  • encode_abi(['string[]'], [[]]) vs Solidity string[] sl; abi.encode(sl);

These discrepancies still yielded correct instructions for the EVM but ultimately led to different hashes and higher gas values for transactions encoded this way.

To-Do

  • Clean up commit history
  • Add entry to the release notes
  • Add cute animal picture

Cute Animal Picture

20220213_153047

@fselmo fselmo force-pushed the eth-abi-vs-solidity-abi-discrepancies branch 6 times, most recently from 212f4c8 to 072486a Compare February 18, 2022 19:50
@fselmo fselmo changed the title [WIP] Investigate and resolve discrepancies with solidity's abi.encode() Investigate and resolve discrepancies with solidity's abi.encode() Feb 18, 2022
@fselmo fselmo marked this pull request as ready for review February 18, 2022 19:57
@fselmo fselmo requested a review from kclowes February 18, 2022 20:17
@fselmo fselmo force-pushed the eth-abi-vs-solidity-abi-discrepancies branch from 072486a to 85fc4f6 Compare February 21, 2022 18:30
@fselmo fselmo requested review from wolovim and pacrob February 21, 2022 18:31
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

lgtm!

@fselmo fselmo force-pushed the eth-abi-vs-solidity-abi-discrepancies branch from 85fc4f6 to b4069f5 Compare February 25, 2022 21:55
- Empty values for dynamic encoders had extra 32-byte zero padding when compared to Solidity's abi.encode() method. This led to correct instructions for the EVM but also required more gas than necessary and ultimately led to differing hashes, etc.

- Add more tests. Decode tuple test values not just as separate list of types but as a tuple type with nested values for encode_abi() tests for tuples.

- Eliminate padding for empty dynamic arrays as well. If an array is static or if it is dynamic and empty do not pad with 32-byte zero padding.
@fselmo fselmo force-pushed the eth-abi-vs-solidity-abi-discrepancies branch from b4069f5 to 75ead60 Compare February 25, 2022 22:06
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.

🎉 Thanks for digging in here!

@kclowes kclowes merged commit 6c09c7f into ethereum:master Mar 2, 2022
@MatthiasLohr
Copy link

Thx!

Any idea when this gets officially released (and even used by web3py)?

@kclowes
Copy link
Contributor

kclowes commented Mar 3, 2022

We'll probably release eth-abi next week sometime, and this will probably make it into web3 v6-beta.2 or beta.3.

@MatthiasLohr
Copy link

No chance of a patch release for web3py 5.28.*? Really struggling issue right now...

@kclowes
Copy link
Contributor

kclowes commented Mar 3, 2022

I'm hesitant to pull it into web3py 5.28 because this is technically a breaking change (though arguably it's a bugfix which is a little less black and white). Most of the breaking changes in web3 v6 right now are dependency-related, so I hope it won't be too bad to update. The other big breaking change is dropping support for python 3.6. Any chance you can try updating to web3 v6 and let me know how it goes? If you run into issues, let me know and I'll consider releasing a patch.

@fselmo fselmo deleted the eth-abi-vs-solidity-abi-discrepancies branch April 4, 2022 22:41
@yixun-alpha
Copy link

Hi, ran into this again on web3py v6 beta (on v3.0.1 of eth-abi); wondering if the above can be merged into v3 of eth-abi? thanks!

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.

encode_abi() does not yield the same result as Solidity's abi.encode() Padding of empty bytes
5 participants