Invalid and inconsistent types in ABIs for library functions

[haltman-at]

Description

While most library functions don't go in the ABI, pure and view functions do. Of course, actually using these is infamously difficult, due to the fact that library selectors are computed differently from other selectors, but they're there. The specific types that differ are structs, enums, and contracts, which in the signature use the internal name of the type, rather than the usual conversion to an ABI type.

The problem is this: In nearly every version of Solidity, including 0.6.10, the ABI parameter entries for enums and contracts put this internal name of the type in the type field, rather than the name of the corresponding ABI type.

Now, this does have the advantage that it allows the selector to be computed correctly by the naive method, without specifically knowing that you're dealing with a library! (As the ABI still lacks anything to indicate the contract/library distinction.) But it has the disadvantage that it screws up any more sophisticated attempt to process the ABI, that is expecting something that conforms to the spec. (Like: You can't actually encode with these types! Without other information, there's nothing to tell us whether a Blank is the name of a contract type or the name of a globally-declared enum type. Web3 and Ethers sure can't handle it, and I'm not about to make Truffle try.)

I suspect this is simply a bug. Further evidence for this is that this only seems to happen to enums and contracts -- structs are unaffected. Even if it's deliberate... well, it should be changed, and the library problem should be handled a different way. (Having internalType goes a long way here, even if we still need something in the ABI to distinguish contracts from libraries.)

Now, on noticing this, I had to check which versions are affected... it turns out that this behavior goes all the way back to 0.1.5, a mere two versions after libraries were introduced in 0.1.3. Given how long this has been around, one might be tempted to say, this isn't a bug, this is just how it works, at least for now. But I'm still calling it a bug: it breaks the spec; it breaks any processing more sophisticated than computing the selector, such as encoding or decoding; and structs don't work this way and have never worked this way, not since 0.4.17 when it first became possible to pass a struct externally.

Environment

• Compiler version: 0.1.5 -- 0.6.10
• Target EVM version (as per compiler settings): default
• Framework/IDE (e.g. Truffle or Remix): Truffle (for 0.4.12 and later); Remix (for 0.4.11 and earlier)

Steps to Reproduce

Try compiling the following:

pragma experimental ABIEncoderV2;

library ABITest {

  struct Pair {
    uint x;
    uint y;
  }

  function structy(Pair calldata) external view { //works fine
  }

  enum Ternary {
    Yes, No, MaybeSo
  }

  function enumy(Ternary) external view { //problem!
  }

  function contracty(Blank) external view { //problem!
  }
}

contract Blank {
}

(comment things out as appropriate to get it to compile with earlier versions; changing calldata to memory and external to public may also be necessary; also changing view to constant (or just omitting that, since everything went in the ABI prior to 0.5.6))

[chriseth]

Yeah, I'm sorry this is really a mess. Someone should create a consistent overview of the current state of affairs.

The ABI for library functions somehow should include which string is used to compute the function selector and potentially how to encode it. One speciality for library functions are of course storage pointers, which are not part of the ABI. So for structy, it makes a big difference if you use Pair calldata or Pair storage and it should result in a different function selector.

[haltman-at]

Yes, sorry, to be clear, I was implicitly omitting functions that take or return storage pointers, since those aren't put in the ABI. I said "pure or view functions", but what I really meant was "pure or view functions that neither take nor return storage pointers".

Once you omit those, then, well... you're left with the fact that these ABIs are not valid. Except for structs. Those ones are valid for some reason. Just not enums and contracts. While I don't know the history here, I can't help but speculate that the decision was made to change it to this broken way in 0.1.5 to make selectors work better, and then this never got revisited, even when structs got added and were done the valid way.

This does of course leave the problem of how to alter the ABI to make computing the selector possible, and there are several possible solutions to that, but I'll skip discussing those because that's a separate issue that's already been much discussed already[0]. Because I think that first of all before that, these invalid ABIs need to be changed to conform to the spec. Sure, tools may get the selector wrong, but that's better than having them, like, just choke on the things. And I mean, who cares if the selector is right if you can't actually encode?

[0]OK I'll state what I see as the two possible solutions briefly. They are:

1. Add a contractKind field to the ABI -- this makes it technically doable by use of internalType (you need some special cases, it's not entirely straightforward, but it's doable; indeed it was while testing some code of mine to do this that this issue came up)
2. Make it so that any function appearing in the ABI gets both the old-style and the standard-style selector

...but again these both have been discussed already elsewhere, so...

[haltman-at]

Also just to be clear -- I think it makes sense to fix this particular issue before sorting out how to handle this problem more generally. Like right now it's malformed and not even consistent (the one benefit of this way is rendered moot by the fact that structs don't work this way -- not that they should (OK, it's also rendered moot by other factors as mentioned above)). May as well handle that first, since that should be pretty easy, and figure out the bigger problem later.

[cameel]

Today I have encountered a bug stemming from the fact that people are generally not aware of this inconsistency (here specifically in case of enums in library ABI): dethcrypto/TypeChain#216

@chriseth So, the current behavior clearly needs to be documented so I'm labeling this as documentation. But do we also consider any part of it a bug? If so, I think the best way to get it done efficiently would be to create smaller and more specific issues for these things.

[cameel]

This actually appears to be a pretty widespread problem. I checked ethers.js and eth-abi (used by Brownie) and none of them handles this properly.

I reported this as bugs (ethers-io/ethers.js#1126, ethereum/eth-abi#146) but if so many tools have problems with it then maybe we should not worry about backwards compatibility here - they don't work with the current ABI anyway.

[haltman-at]

This actually appears to be a pretty widespread problem. I checked ethers.js and eth-abi (used by Brownie) and none of them handles this properly.

I reported this as bugs (ethers-io/ethers.js#1126, ethereum/eth-abi#146) but if so many tools have problems with it then maybe we should not worry about backwards compatibility here - they don't work with the current ABI anyway.

I just want to point out that this is basically the same point I made above. :) :P

[ricmoo]

Is there is actually anyway that ethers can handle this? Is the information required encoded some other way in the ABI?

[cameel]

@haltman-at Right. I read that comment a while ago and now missed that point. In any case, I agree with you. I was just investigating a different issue related to enums and while doing it I also checked what the situation is regarding this one. And it confirms what you said - it's bad :(.

@ricmoo I don't think it is, but @chriseth knows more about the ABI so maybe he'd be able to suggest something. In case of enums people from TypeChain hard-coded it to uint8 for now (dethcrypto/TypeChain#216) but that's not great because enums with more than 256 entries are not uint8. But to avoid it you'd probably have to somehow figure out how many entries the enum has.

We're going to limit enums to uint8 soon (#10035) but it won't change the situation for older compiler versions. Hard-coding it might be a lesser evil given how rare big enums probably are.

[haltman-at]

Note that the type field, unlike the internalType field, just contains the name of the contract or enum; it doesn't include enum or contract or interface in front of it. So you'd have to look at the internalType field to determine wheter you're looking at an enum or a contract.

Hm, I suppose this actually might be more handleable than I thought, given that:

1. On 0.5.11 or later, you can use internalType as described;
2. On 0.5.10 or earlier, there were no top-level enums, so you can distinguish a contract from an enum by whether there's a period in the name;
3. And then enums can generally be assumed to be uint8 even if that doesn't work all the time.

That's pretty hacky. I mean, it's the sort of thing I might just do, but probably not the sort of thing I'd expect anyone else to do. :)