rust-ethereum / ethabi

Encode and decode smart contract invocations
Apache License 2.0
517 stars 186 forks source link

Ethabi cannot parse library with public function #254

Closed gakonst closed 2 years ago

gakonst commented 2 years ago

Problem

It seems that ethabi cannot parse the ABI of a library contract with a public function (i.e. one that's intended to be linked)

This was originally reported in https://github.com/gakonst/foundry/issues/349.

Repro

pragma solidity >=0.7.6;

library LibTest {
    enum TestEnum {
        A,
        B,
        C
    }

    function foo(TestEnum test) public view {}
}

solc contract.sol --abi generates:

[{"inputs":[{"internalType":"enum LibTest.TestEnum","name":"test","type":"LibTest.TestEnum"}],"name":"foobar","outputs":[],"stateMutability":"view","type":"function"}]

And then doing

let abi = r#"[{"inputs":[{"internalType":"enum LibTest.TestEnum","name":"test","type":"LibTest.TestEnum"}],"name":"foobar","outputs":[],"stateMutability":"view","type":"function"}]
"#;
let abi: ethabi::Contract = serde_json::From_str(abi).unwrap();

panics with:

panicked at 'called `Result::unwrap()` on an `Err` value: Error("InvalidName(\"LibTest.TestEnum\")", line: 1, column: 167)

Diagnosis

The error originates in this match branch. Custom type names are seemingly not supported.

let result = match name {
            "address" => ParamType::Address,
            "bytes" => ParamType::Bytes,
            "bool" => ParamType::Bool,
            "string" => ParamType::String,
            "int" => ParamType::Int(256),
            "tuple" => ParamType::Tuple(vec![]),
            "uint" => ParamType::Uint(256),
            s if s.starts_with("int") => {
                let len = s[3..].parse()?;
                ParamType::Int(len)
            }
            s if s.starts_with("uint") => {
                let len = s[4..].parse()?;
                ParamType::Uint(len)
            }
            s if s.starts_with("bytes") => {
                let len = s[5..].parse()?;
                ParamType::FixedBytes(len)
            }
            _ => {
                return Err(Error::InvalidName(name.to_owned()));
            }

Solution

Can we assume that custom names that are not tuples are enums? If so, it could make sense to do _ => ParamType::Uint(8). I'm not sure that assumption is correct. It'd be nice if we could access the internalType inside Reader, since it contains the word enum.

nlordell commented 2 years ago

Weird, there seems to be an inconsistency of ABI generated for a library vs a contract. Specifically if you s/library/contract then it generates an ABI with:

[
  {
    "inputs": [
      {
        "internalType": "enum ContractTest.TestEnum",
        "name": "test",
        "type": "uint8"
      }
    ],
    "name": "foo",
    "outputs": [],
    "stateMutability": "view",
    "type": "function"
  }
]

Can we assume that custom names that are not tuples are enums? If so, it could make sense to do _ => ParamType::Uint(8).

I think that's fine. However, I think we should first check if there are any other custom types that affect the type field in this way. Some that come to mind:

gakonst commented 2 years ago

Structs get converted to tuple I believe, so they are caught earlier in the match statement.

User defined value types are zero-cost, so they all compile down to the native type (e.g. type A is uint256) generates uint256 in the ABI.

I think we might be able to get away with this - cc @mattsse

mattsse commented 2 years ago

If so, it could make sense to do _ => ParamType::Uint(8).

this seems appropriate, but then every unknown identifier will be treated as uint8.

does it make sense to make two modes, like lenient and strict? as it's already used for the tokenziers

gakonst commented 2 years ago

Yeah I guess the question is: Are there unknown identifiers which may NOT be uint8s?

nlordell commented 2 years ago

Are there unknown identifiers which may NOT be uint8s?

My worry was that something like this:

pragma solidity >=0.8.10;
library L {
    type T is uint256
    function test(T t) public view {}
}

Would generate an ABI with:

[
  {
    "inputs": [
      {
        "internalType": "Test.Type",
        "name": "t",
        "type": "Test.Type"
      }
    ],
    "name": "test",
    "outputs": [],
    "stateMutability": "view",
    "type": "function"
  }
]
The good news, is that it doesn't: ```solidity // SPDX-License-Identifier: 0BSD pragma solidity 0.8.11; library Test { enum Enum { A } struct Struct { uint256 a; } type Type is uint256; function test(Enum x, Struct calldata y, Type z) public view {} } ``` ```json [ { "inputs":[ { "internalType":"enum Test.Enum", "name":"x", "type":"Test.Enum" }, { "components":[ { "internalType":"uint256", "name":"a", "type":"uint256" } ], "internalType":"struct Test.Struct", "name":"y", "type":"tuple" }, { "internalType":"Test.Type", "name":"z", "type":"uint256" } ], "name":"test", "outputs":[ ], "stateMutability":"view", "type":"function" } ] ``` So I think it should be OK to do this workaround. Maybe this is something we can point this out to the Solidity team as it _feels_ like a `solc` bug.
gakonst commented 2 years ago

cc @chriseth maybe can help here

mattsse commented 2 years ago

from the abi-spec docs about the type field, which is "the canonical type of the parameter"

The canonical type is determined until a tuple type is reached and the string description up to that point is stored in type prefix with the word tuple

https://docs.soliditylang.org/en/v0.8.10/abi-spec.html#handling-tuple-types

which explains why structs are always type: tuple, and user defined value types (type A is B) are merely aliases so their canonical type is their actual type (B).

but this doesn't explain why an enum is type: uint8 in contracts and type: <name> in libraries. judging by the docs it seems like it should be always type: <name> but since they're uint8 it would make more sense to always resolve them to uint8. This feels like an inconsistency on solc's part.

gakonst commented 2 years ago

Personally fine with us moving forward with assuming it's all u8's, think the fastest way to close the loop here is to make the change and find the edge downstream, if it exists. Any outstanding concerns on your end, @nlordell ?

chriseth commented 2 years ago

Public library functions are not meant to be called "from outside". This means the calling convention of public library functions is considered to be solidity-internal, in other words, they actually don't even have an ABI.

The effect is that the function hashes of library functions differ from function hashes of contract functions. IIRC the main differences are:

the same applies recursively for compound types