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

Dynamic types encoding doesn't conform to spec #22

Closed
pdobacz opened this issue Dec 6, 2019 · 7 comments · Fixed by #34
Closed

Dynamic types encoding doesn't conform to spec #22

pdobacz opened this issue Dec 6, 2019 · 7 comments · Fixed by #34

Comments

@pdobacz
Copy link

pdobacz commented Dec 6, 2019

By spec I mean this: https://solidity.readthedocs.io/en/v0.5.13/abi-spec.html#use-of-dynamic-types

In particular:

whereas for the dynamic types uint32[] and bytes, we use the offset in bytes to the start of their data area, measured from the start of the value encoding

There was one test that touched this encoding/decoding, but it used the "decodes to binary that it encoded to" assertion, so it passed tests.

I suspect this worked before #14, but got missed there and removed. Now decoding "forgets" the "start of their data area" so it is not possible with the decode_... functions currently existing.

To spin this up, I added the tests for type_decoder to cover this and I did some tidies to facilitate that. Now there's a separation between tests that only use encode |> decode to test, tests leveraging examples from the spec and tests running on own data examples. New tests fail with exceptions, because things don't pattern match inside the guts of decoding. These tests are in a branch:

https://github.com/omisego/ex_abi/tree/fix_dynamic_arrays_decoding

It would be ideal to have the fix for the problem branch off of this branch/commit, so that the tests listed can start to pass and be kept.

(this branch with tests doesn't remove any of the existing tests, at most does some cosmetic refactors and moves them around)

Steps to reproduce

Checkout the branch ☝️ and mix test

@InoMurko
Copy link
Contributor

InoMurko commented Jan 31, 2020

Tried wrapping my head around the code and it was rather difficult...
This is also related to, because strings stopped working. #14

For example, strings as dynamic types are incorrectly encoded and decoded.
#15

And in a set of types, like:

  test "with multiple types" do
      encode_result =
        "00000000000000000000000000000000000000000000000000000000000001230000000000000000000000000000000000000000000000000000000000000080313233343536373839300000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e0000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000004560000000000000000000000000000000000000000000000000000000000000789000000000000000000000000000000000000000000000000000000000000000d48656c6c6f2c20776f726c642100000000000000000000000000000000000000"

      types = [
        {:uint, 256},
        {:array, {:uint, 32}},
        {:bytes, 10},
        :bytes
      ]
      result = [0x123, [0x456, 0x789], "1234567890", "Hello, world!"]
      assert encode_result == Base.encode16(TypeEncoder.encode(result, types), case: :lower)
      assert result == TypeEncoder.encode(result, types) |> TypeDecoder.decode(types)
    end

results are not correctly padded.

@vbaranov
Copy link
Collaborator

@pdobacz, 0.3.0 just issued (https://github.com/poanetwork/ex_abi/releases/tag/0.3.0). It should fix issues, that you noticed. Do you still experience incorrect decoding/encoding of dynamic-size types?

@pdobacz
Copy link
Author

pdobacz commented Mar 19, 2020

@vbaranov Hi, cool, thank you! I've seen you've extended tests in that PR. Did you use the tests proposed in this issue? (https://github.com/omisego/ex_abi/commits/fix_dynamic_arrays_decoding)

@InoMurko
Copy link
Contributor

@vbaranov it seems like .3. wasn't pushed to hex.

@unnawut
Copy link

unnawut commented Apr 13, 2020

@vbaranov Thank you for the fix! Like above, I can confirm that 0.3.0 is not on Hex yet: https://hex.pm/packages/ex_abi

@ayrat555
Copy link
Collaborator

https://hexdocs.pm/ex_abi/0.3.1

@ayrat555
Copy link
Collaborator

ayrat555 commented May 23, 2020

I created a PR that adds a bunch of fixes including a complete re-write of the encoder. @InoMurko @vbaranov @pdobacz @unnawut #34
Can you please verify that it works seamlessly?

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 a pull request may close this issue.

5 participants