poanetwork / ex_abi

The Ethereum ABI Interface
GNU General Public License v3.0
61 stars 43 forks source link

`ex_abi` can't decode `string` type #15

Closed ayrat555 closed 4 years ago

ayrat555 commented 5 years ago

The issue is related to the latest PR (https://github.com/poanetwork/ex_abi/pull/14). Now data byte length is not considered for string type (maybe for some other dynamic types)

volnyvolnyvolny commented 5 years ago

@ayrat555, could you please clarify your statement?

Maybe I missed something, but I see nothing criminal in the decoding of the string type. I hoped that random testing would show that when decoding some previously encoded values it will not match the original but had no luck in this direction (PR https://github.com/poanetwork/ex_abi/pull/16).

ayrat555 commented 5 years ago

@zergera try setting the latest version of ex_abi to blockscout. you'll a bunch of tests failing. they're related to string encoding

volnyvolnyvolny commented 5 years ago

@ayrat555, yes, setting version of ex_abi to 0.2.0 produces tests failings. What I see at the moment is that in all the failed tests there is exactly one pattern: you expect [string] type, but compare it to the value of [(string)].

For ex.:

#ethereum_jsonrpc
test "correctly decodes string types" do
  result =
    "0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000441494f4e00000000000000000000000000000000000000000000000000000000"

# either use:
# result =
  # "0x000000000000000000000000000000000000000000000000000000000000000441494f4e00000000000000000000000000000000000000000000000000000000"

  selector =
    %ABI.FunctionSelector{
        function: "name",
        types: [],
        returns: [:string]

        # or:
        # returns: [{:tuple, [:string]}]
    }

  assert Encoder.decode_result(%{id: "storedName", result: result}, selector)
      == {"storedName", {:ok, ["AION"]}}
  # and:
  #   == {"storedName", {:ok, [{"AION"}]}}
end

So we either use

0x0000000000000000000000000000000000000000000000000000000000000004 # number of bytes
  41494f4e00000000000000000000000000000000000000000000000000000000 # "AION"

as a result string (without offset that makes it (string) type), or expect every string result to be wrapped in a tuple (like [{"AION"}]).

As so, I think that it is not an issue of the ex_abi library. This library decodes string exactly as it should. The problem is in the interpretation of the function call result in the blockscout ethereum_jsonrpc component when it's string.

Of course, a bypass clause can be added to the apps/ethereum_jsonrpc/lib/ethereum_jsonrpc/encoder.ex:

def decode_result(result, %{returns: [:string]} = fs) do
  case decode_result(result, %{fs | returns: [{:tuple, [:string]}]}) do
    {id, {:ok, [{string}]}} ->
      {id, {:ok, [string]}}

    els ->
      els
  end
end

but usually this is not the best idea, and it is not related to the ex_abi.

As so, I suggest to close this issue and continue this discussion in the https://github.com/poanetwork/blockscout/issues/1546.

ayrat555 commented 5 years ago

@zergera I think [string] vs [(string)] is exactly the problem of ex_abi library. Because we can't change abi interfaces of smart contracts

volnyvolnyvolny commented 5 years ago

@ayrat555.

At the moment, your library follows the letter of the specification, except for the decoding of the address type. According to the specification,

0x0000000000000000000000000000000000000000000000000000000000000004
  41494f4e00000000000000000000000000000000000000000000000000000000

is the encoded representation of the value "AION" (type string).

And

0x0000000000000000000000000000000000000000000000000000000000000020
  0000000000000000000000000000000000000000000000000000000000000004
  41494f4e00000000000000000000000000000000000000000000000000000000

is the encoded representation of the value ("AION") (type (string)).

If you'll try to decode the second byte sequence as a value of type string,

0x0000000000000000000000000000000000000000000000000000000000000020

will be interpreted as a byte length of the following string (32 bytes) and

0000000000000000000000000000000000000000000000000000000000000004

as a string itself.

  41494f4e00000000000000000000000000000000000000000000000000000000

will be considered as the next data piece. If you decide to change the decoding behavior, you will rely on the existence of this tail (rest) data. It doesn't feel like good practice.

ayrat555 commented 5 years ago

@zergera can you please create PR in blockscout fixing all errors?

volnyvolnyvolny commented 5 years ago

@ayrat555, quick fix is https://github.com/poanetwork/blockscout/pull/1824 .

vbaranov commented 4 years ago

Decoding of string types fixed in 0.3.0 version of library https://github.com/poanetwork/ex_abi/pull/24