Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

Out-of-date handling of bytes and string arrays -- should this repo be marked as deprecated? #86

Closed
alex-miller-0 opened this issue Feb 11, 2021 · 2 comments

Comments

@alex-miller-0
Copy link

It appears that the ABI spec changed in 2019 or 2020 and this code base has not kept up with it. As a result, all arrays of bytes or string types are encoded incorrectly by this library.

Consider the following toy example with types [ 'bytes[]', 'string[]' ] and values:

[
  [ [ 0x12, 0x34, 0x56, 0x78 ] ],
  [ 'one' ],
]

I have encoded this using ethers and this repo. The results diverge:

ethers:

0000000000000000000000000000000000000000000000000000000000000040    // offset of bytes[] param
00000000000000000000000000000000000000000000000000000000000000c0    // offset of string[] param
0000000000000000000000000000000000000000000000000000000000000001    // number of bytes[] elements
0000000000000000000000000000000000000000000000000000000000000020    // offset of first bytes[] element
0000000000000000000000000000000000000000000000000000000000000004    // size of first bytes[] element
1234567800000000000000000000000000000000000000000000000000000000    // first bytes[] element
0000000000000000000000000000000000000000000000000000000000000001    // number of string[] elements
0000000000000000000000000000000000000000000000000000000000000020    // offset of first string[] element
0000000000000000000000000000000000000000000000000000000000000003    // size of first string[] element
6f6e650000000000000000000000000000000000000000000000000000000000    // first string[] element

ethereumjs-abi:

0000000000000000000000000000000000000000000000000000000000000040    // offset of bytes[] param
00000000000000000000000000000000000000000000000000000000000000a0    // offset of string[] param
0000000000000000000000000000000000000000000000000000000000000001    // number of bytes[] elements
0000000000000000000000000000000000000000000000000000000000000004    // size of first bytes[] element
1234567800000000000000000000000000000000000000000000000000000000    // first bytes[] element
0000000000000000000000000000000000000000000000000000000000000001    // number of string[] elements
0000000000000000000000000000000000000000000000000000000000000003    // size of first string[] element
6f6e650000000000000000000000000000000000000000000000000000000000    // first string[] element

Note the additional offsets for each item in the array params in the ethers result. While I don't understand why the spec changed (there is no new information in the ethers result -- just more data describing offsets that don't seem necessary and pollute blockspace -- but that's none of my business...), I have validated that the ethers result is correct using mainnet transactions with known schema.

Since there have been no new code commits to this repo in 2 years I'm guessing it was abandoned. If that's the case, it would be helpful if you could mark it as deprecated in the README. This would avoid people like me using it as a reference implementation only to learn it was invalid for certain types.

alex-miller-0 added a commit to GridPlus/ethereum-abi-c that referenced this issue Feb 11, 2021
We were using an out-of-data reference implementation that does not correctly
support bytes and string arrays. This updates to the latest ABI spec and
swaps in test vectors generated by a new reference implementation that is
correct.
For more info, see: ethereumjs/ethereumjs-abi#86
alex-miller-0 added a commit to GridPlus/ethereum-abi-c that referenced this issue Feb 11, 2021
We were using an out-of-data reference implementation that does not correctly
support bytes and string arrays. This updates to the latest ABI spec and
swaps in test vectors generated by a new reference implementation that is
correct.
For more info, see: ethereumjs/ethereumjs-abi#86
alex-miller-0 added a commit to GridPlus/ethereum-abi-c that referenced this issue Feb 11, 2021
We were using an out-of-data reference implementation that does not correctly
support bytes and string arrays. This updates to the latest ABI spec and
swaps in test vectors generated by a new reference implementation that is
correct.
For more info, see: ethereumjs/ethereumjs-abi#86
@holgerd77
Copy link
Member

Yes, yes, you are so right. We decided quite some time ago that we want to deprecate here and point to Ethers instead but never executed on it. Will directly do and deprecate the library.

Sorry for this delay - also as some placeholder for anyone who had unnecessary troubles - and thanks a lot for this reminder! 😄

@holgerd77
Copy link
Member

Ok, added a deprecation note to the README, I tried to give some good summary on the circumstances, you might want to give it a read and give some short feedback if everything is correct.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants