Consolidate encoding and decoding methods, rename

[fselmo]

What was wrong?

• encode_single() / decode_single() and encode_abi() / decode_abi() have very similar functions. It has been suggested that these APIs be consolidated into one and the changes in this PR reflect keeping the encode_abi() and decode_abi() functionality and renaming the methods to encode() and decode(), respectively. encode_abi_packed() has also been changed to encode_packed() - as is Solidity's.

• I also changed, in a separate commit, the order in which the arguments are passed to these methods to be: values first; types second. This better mimics Solidity's abi methods for consistency and since these changes are breaking and significantly change the methods, this felt like a good time to make such a change. I'm also not tied to this idea and welcome any input if anyone feels strongly about not making this change.

Solidity

bytes32 a = 0x6100000000000000000000000000000000000000000000000000000000000000;
bytes32 b = 0x6200000000000000000000000000000000000000000000000000000000000000;
// abi.encode()
bytes public encoded_data = abi.encode(a, b);

function returnEncodedData() public view returns (bytes32[2] memory) {
    // can locally unpack encoded data with abi.decode() 
    (bytes32 decoded_a, bytes32 decoded_b) = abi.decode(encoded_data, (bytes32,bytes32));
    
    return [decoded_a, decoded_b];
}

Python

>>> from eth_abi import encode, decode

>>> # abi.encode()
>>> encoded_data = encode((b'a', b'b'), ('bytes32', 'bytes32'))
>>> encoded_data
b'a\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00b\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'

>>> # unpack encoded data in a similar way to Solidity example above
>>> (decoded_a, decoded_b) = decode(encoded_data, ('bytes32', 'bytes32'))
>>> decoded_a
b'a\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
>>> decoded_b
b'b\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'

closes #84

How was it fixed?

• Removed encode_single() and decode_single() methods.
• Renamed encode_abi() to encode() and decode_abi() to decode().
• Fixed / refactored tests to accommodate for the above changes, including changing the encode_single() / decode_single() tests to use encode() and decode() with a single type passed in.
• Some minor cleanup along the way where there were unused imports, etc.

To-Do

• Clean up commit history
• Consider reversing the arguments to the methods to even closer mimic the Solidity abi library methods (i.e. abi.decode(encoded_data, ('bytes32', 'bytes32'))).
• Add entry to the release notes
• Add cute animal picture

Cute Animal Picture

[kclowes]

Code looks really solid!

• I could take or leave the argument order change. It looks like we will need to update/make breaking changes in web3.py, eth-tester, and eth-account, and maybe others that I missed. Github isn't correctly picking up libraries that depend on eth-abi for some reason.
• I think the API would be a little bit nicer if we could pass arguments either with or without a list, for example, allowing: encode(234, 'uint'), but feel free to take or leave that feedback :)

We should also talk about whether or not we should do a beta release for these changes, but we can talk about that in our sync on Monday! 💥

[kclowes]

Oh! I forgot to mention that we'll need to cut a v3 stable branch and deprecate the methods in that branch. So let's wait to merge this until I have a v3 stable branch out. I'll circle back and post a comment here once that's up!

[charles-cooper]

FWIW I'd say keeping the existing argument order makes some sense. I think the existing way works well if you think in terms of currying - the first argument is "more slowly" changing, the second argument "more quickly". Plus, and maybe this is more important, downstream users don't need to change. Trying to figure out what abi_encode("string", "(string)") encodes to will require less mental gymnastics, for instance.

[fselmo]

FWIW I'd say keeping the existing argument order makes some sense. I think the existing way works well if you think in terms of currying - the first argument is "more slowly" changing, the second argument "more quickly". Plus, and maybe this is more important, downstream users don't need to change. Trying to figure out what abi_encode("string", "(string)") encodes to will require less mental gymnastics, for instance.

@charles-cooper thanks for the feedback. I could also take or leave it but figured we could consider it given we're already introducing breaking changes. It did feel like it made more sense to me reversed / the way it is in Solidity: "decode this data as these types". For better or worse I do think leaving the current argument order would cause less headaches and maybe we should just leave it alone.

[kclowes]

LGTM! The only thing that sticks out to me while looking over this again is: what happens if type_str is passed without an array? Is the error clear enough or should we add our own? I was thinking about it in the context of people migrating from v2/v3 🤷

[fselmo]

LGTM! The only thing that sticks out to me while looking over this again is: what happens if type_str is passed without an array? Is the error clear enough or should we add our own? I was thinking about it in the context of people migrating from v2/v3 🤷

Updated with some better messaging and tests in this commit. Good thought. Could you do another pass through and let me know?

[kclowes]

LGTM! 🚢