Abiv2

[vkgnosis]

Edit: Ready for review, scroll down for update.

Nick pointed out that there is an ethabi fork that already supports abiv2. This PR is using this.

Where I am currently stuck is on the tokenization for web3. Web3 has several related traits https://docs.rs/web3/0.15.0/web3/contract/tokens/index.html . The trait that is usually accepted and used by ethcontract is Tokenize.
In my solidity example we have

struct S {
  uint8 u0;
  uint16 u1;
}

function takeStruct(S calldata s) public view {}

In the abi this becomes Tuple(uint8, uint16) which gets turned by ethcontract in this PR into s : (u8, u16).

Web3 has one trait Tokenizable for things that can get turned into a single Token and another trait Tokenize that turns things into Vec<Token>. The latter is implemented probably for convenience so that users of web3 can pass tuples of different tokenziable things to it. This has now become problematic because it prevents tuples of Tokenizable to themself implement Tokenizable (by turning into Token::Tuple) because of ambiguous implementations. Imo the correct way to fix this is to get rid of the Tokenize trait so that we can have the mentioned tuple impl. This involves getting this merged in web3 first.
I am not sure if there is a way to achieve the same thing without touching web3. The only alternative I see is to duplicate Tokenize into ethcontract and implemented it correctly for tuples and then pass our Tokenize around instead of web3's.

[fleupold]

I don't fully understand the issue (I checked out the code but couldn't find concrete errors).

Tokenizable requires two methods: from_token and into_token.

Tokenize requires one method: into_tokens. Where is the ambiguous implementation?

What I don't understand is how you would be able to implement Tokenizable on the raw Tuple as neither the type not the trait are local to your crate? Wouldn't we have to add this implementation to web3?

[vkgnosis]

Probably not worth your time to investigate too deeply, Felix. I was mostly thinking out loud in case @nlordell had encountered this before. Will try to explain it again:
Web3 has a trait Tokenizable that is implemented for basic types like u8, String, etc. The trait is also implemented for slices of basic types because those can map to Token::Array. In the same vein you would expect the trait to be implemented for tuples of itself because those map to Token::Tuple.
But actually web3 has a different trait Tokenize that is implemented on tuples of Tokenizable. The previous trait is able to turn T into a single Token and this trait is able to turn T into Vec. I think the reason for having this is to make it easy to pass tokens to representing multiple function arguments. The problem is that with this setup if tuples of Tokenizable were themself Tokenizable it would be ambiguous whether they would encode into a single Token::Tuple or a Vec. This is why tuples of Tokenizable aren't and cannot be Tokenizable. Which means with web3's api you cannot pass something like ((u8; u16); u32) because (u8; u16) is not Tokenizable so the whole thing is not Tokenizable.
As you say fixing this properly requires a change in web3. Another solution I see now is that we can make our own Tokenizable trait that doesn't have these issues. That might be useful as an immediate fix while discussion on what is best to do in web3 continues in form of a PR there.

[nlordell]

I was mostly thinking out loud in case @nlordell had encountered this before.

This rings a bell. I think this is why I abandoned implementing this a while back.

I think the issue is that the web3 traits are insufficient for modelling nested Tokenizable doesn't support nesting as you described.

[vkgnosis]

I am working on a PR for web3 that would fix this but it has to introduce some breakage there. Until that is merged (if ever) we should be able to duplicate the same trait changes into ethcontractract because we manually call ethabi::encode / decode anyway without going through web3 for that, so we don't have to use the same trait to create Tokens as web3 does.

[vkgnosis]

I've moved the tokenizaton related traits from rust-web3 into ethcontract. This makes them simpler (than the rust-web3 version) because ethcontract uses them for fewer purposes than rust-web3 and gives us more control over them. I feel this is what we should do mid-term until we see the route web3 and ethabi take.

Still WIP because I need to add error handling, documentation and more tests for abi v2.

[vkgnosis]

This is ready for review now. PR is big but I don't see a way to split this into semantically useful parts. 80% of the changes are the new file tokens.rs which itself mostly comes from rust-web3. I recommend reading the commit message too for some context.

You can see the change I had to make to ethabi here https://github.com/vkgnosis/ethabi/commits/ethcontract .

[fleupold]

Just some comments, as I'm still not sure I fully understand what's going on. Leaving the thorough review up to @nlordell

[nlordell]

In general, the code looks good.

I still haven't wrapped my head around the SingleTokenize and MultiTokenize traits. I'm going to take a second look with fresh eyes on Monday.

[nlordell]

I found an old PR for adding support for decoding nested tuples:
rust-ethereum/ethabi#186

[nlordell]

Code looks good to me.

It might be worth exploring whether the MultiTokenize can just be represented by a SingleTokenize that always returns a tuple, this way we have one less trait.

I think the distinction was important in web3 prior to ABI v2 as function parameters are kind of like a tuple, but nested tuples weren't allowed, but since this is no longer the case, we might get away with a single trait.

[vkgnosis]

Added two new commits that simplify the code by removing MultiTokenize and Return. It turns out we can represent both cases as SingleTokenize tuples.

[vkgnosis]

The one remaining point is what to do about the ethabi fork. As Nick pointed out we can't use the git dependency because it would prevent ethcontract from being publishes to crates.io. I think a reasonable short term fix is if I publish a version of my fork so we can have it as a crates.io dependency. Long term we can still depends on the original ethabi crate.

[nlordell]

Amazing! I think it turned out really good!

[vkgnosis]

@nlordell FYI I had to add another small commit (the most recent one) here for a problem I discovered when using this branch in v2-services. It's a minor change and I am probably going to merge this PR tomorrow.

[nlordell]

Changes look fine to me.