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

[Flow EVM] Add support for ABI encoding/decoding of Solidity byte types #6553

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Oct 10, 2024

Closes: #5365

Support ABI encoding/decoding the following Solidity byte types:

  • bytes & bytes[]
  • bytes4 & bytes4[]
  • bytes32 & bytes32[]

Note: A contract upgrade for EVM system contract is needed.

The test payloads have been generated with the help of: https://abi.hashex.org/

@m-Peter m-Peter force-pushed the add-abi-support-for-solidity-byte-types branch from caf56eb to d004ec5 Compare October 10, 2024 12:11
@m-Peter m-Peter changed the title Add ABI encoding/decoding support for Solidity byte types Add support for ABI encoding/decoding of Solidity byte types Oct 10, 2024
@m-Peter m-Peter marked this pull request as ready for review October 10, 2024 13:24
@m-Peter m-Peter marked this pull request as draft October 10, 2024 13:35
@m-Peter m-Peter marked this pull request as ready for review October 10, 2024 14:38
@m-Peter m-Peter changed the title Add support for ABI encoding/decoding of Solidity byte types [Flow EVM] Add support for ABI encoding/decoding of Solidity byte types Oct 10, 2024
@turbolent
Copy link
Member

Great work adding support for more types!

I had a look at the documentation at https://docs.soliditylang.org/en/latest/types.html#fixed-size-byte-arrays, and from what I understood, the Solidity bytes type (dynamic-sized byte array) is equivalent to the Cadence type [UInt8] (variable-sized array), the bytesN type (fixed-size byte-array) is equivalent to the Cadence type [UInt8; N] (constant-sized array).

Why do we need the "wrapper types", like EVM.EVMBytes4?

@m-Peter
Copy link
Collaborator Author

m-Peter commented Oct 11, 2024

Great work adding support for more types!

I had a look at the documentation at https://docs.soliditylang.org/en/latest/types.html#fixed-size-byte-arrays, and from what I understood, the Solidity bytes type (dynamic-sized byte array) is equivalent to the Cadence type [UInt8] (variable-sized array), the bytesN type (fixed-size byte-array) is equivalent to the Cadence type [UInt8; N] (constant-sized array).

Why do we need the "wrapper types", like EVM.EVMBytes4?

@turbolent Let me explain this with a working example, to illustrate the differences:

let bytes4 = '0x050a0f14' // these are the values 5, 10, 15, 20 in hex
let abiEncoded = web3.eth.abi.encodeParameter('bytes4', bytes4)
assert.equal(
    abiEncoded,
    '0x050a0f1400000000000000000000000000000000000000000000000000000000'
)
    
let uint8four = [5, 10, 15, 20] // this is the same value as `bytes4` variable above
abiEncoded = web3.eth.abi.encodeParameter('uint8[4]', uint8four)
assert.equal(
    abiEncoded,
    '0x0000000000000000000000000000000000000000000000000000000000000005000000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000f0000000000000000000000000000000000000000000000000000000000000014'
)

let bytes = '0x050a0f14'
abiEncoded = web3.eth.abi.encodeParameter('bytes', bytes)
assert.equal(
    abiEncoded,
    '0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000004050a0f1400000000000000000000000000000000000000000000000000000000'
)

The Solidity bytes-related types, are probably used for compact encoding, compared to Solidity arrays.

Right now, we have mapped the Cadence dynamic-sized & fixed-sized arrays, to Solidity arrays:

  • [UInt8] -> uint8
  • [UInt8; 4] -> uint8[4]

Which means that we need to somehow control the ABI encoding/decoding of Solidity bytes-related types. That's why I've introduced the wrapper types.

  • EVM.EVMBytes -> bytes
  • EVM.EVMBytes4 -> bytes4
  • EVM.EVMBytes32 -> bytes32

These are the most commonly used Solidity bytes-related types. I have also added support for arrays of those types. Let me know if this makes sense 🙏

@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 88.15789% with 27 lines in your changes missing coverage. Please review.

Project coverage is 41.33%. Comparing base (3ba181e) to head (f067110).

Files with missing lines Patch % Lines
fvm/evm/impl/abi.go 82.01% 17 Missing and 8 partials ⚠️
utils/unittest/execution_state.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6553      +/-   ##
==========================================
+ Coverage   41.28%   41.33%   +0.05%     
==========================================
  Files        2031     2031              
  Lines      145883   146072     +189     
==========================================
+ Hits        60222    60385     +163     
- Misses      79442    79461      +19     
- Partials     6219     6226       +7     
Flag Coverage Δ
unittests 41.33% <88.15%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@turbolent
Copy link
Member

Thank you for the explanation, it makes sense now! Initially I assumed that the byte types were just aliases, but I understand now that they are independent types, to allow for more compact encoding.

Ideally we would probably like to have a type like EVM.EVMBytes<N>, which contains a [UInt8; N], and where N is a compile-time constant. However, Cadence does not support generic types in Cadence code yet.

The code looks good, and adding support for the common types makes sense. Maybe we can consider generating the code for all N 1...32 with go:generate, but we can do that later, if needed

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

fvm/evm/impl/abi.go Outdated Show resolved Hide resolved
@turbolent
Copy link
Member

As the EVM contract change, the state commitments need to be updated as well. You can just use the new reported values as the expected values.

@m-Peter m-Peter force-pushed the add-abi-support-for-solidity-byte-types branch from 0f7e0b1 to f067110 Compare October 16, 2024 08:20
@m-Peter
Copy link
Collaborator Author

m-Peter commented Oct 16, 2024

As the EVM contract change, the state commitments need to be updated as well. You can just use the new reported values as the expected values.

Updated in f067110

@bluesign
Copy link
Contributor

bluesign commented Oct 16, 2024

Ideally we would probably like to have a type like EVM.EVMBytes, which contains a [UInt8; N], and where N is a compile-time constant. However, Cadence does not support generic types in Cadence code yet.

Out of curiosity, can we do something like EVM.createBytes<N>([UInt8;N]): EVM.EVMBytes<N> and EVM.createBytes([UInt8]): EVM.EVMBytes ?

@turbolent
Copy link
Member

@bluesign

Out of curiosity, can we do something like EVM.createBytes<N>([UInt8;N]): EVM.EVMBytes<N> and EVM.createBytes([UInt8]): EVM.EVMBytes ?

That would be nice, and the ideal solution. However, Cadence does not support generic types defined in Cadence programs yet, and further, it only supports generic parameters which are types at the moment, there is no support for generic parameters like numeric constants yet (see e.g. "const generics" in Rust)

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Great work! 👏

Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Nice!

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.

[Flow EVM] Add support for ABI encoding/decoding of Solidity bytes -related types
5 participants