ABIGEN v2

[s1na]

This PR adds a new version of abigen which will co-exist parallel to the existing version for a while. To generate v2 bindings provide the --v2 flag to abigen cmd.

Summary

The main point of abigen v2 is having a lightweight generated binding which allows for more composability. This is possible now thanks to Go generics. "only" methods to pack and unpack call and return data are generated for a contract. As well as unpacking of logs.

To interact with the contract a library is available with functions such as Call, Transact, FilterLogs, etc. These take in the packed calldata, or a function pointer to unpack return data.

Features

The new version is at feature-parity with v1 at a much lower generated binding size. The only missing feature as of now is sessions.

Example

V1 and v2 bindings for a sample contract are available here: https://gist.github.com/s1na/05f2d241b07372b41ba1747ce6e098b7. A sample script using v2 is available in main.go.

[MariusVanDerWijden]

Something is broken here:

func (_{{$contract.Type}} *{{$contract.Type}}) Unpack{{.Normalized.Name}}(data []byte) ({{if .Structured}}struct{ {{range .Normalized.Outputs}}{{.Name}} {{bindtype .Type $structs}};{{end}} },{{else}}{{range .Normalized.Outputs}}{{bindtype .Type $structs}},{{end}}{{end}} error) {
			out, err := _{{$contract.Type}}.abi.Unpack("{{.Original.Name}}", data)
			{{if .Structured}}
			outstruct := new(struct{ {{range .Normalized.Outputs}} {{.Name}} {{bindtype .Type $structs}}; {{end}} })
			if err != nil {
				return *outstruct, err
			}
			{{range $i, $t := .Normalized.Outputs}} 
			outstruct.{{.Name}} = *abi.ConvertType(out[{{$i}}], new({{bindtype .Type $structs}})).(*{{bindtype .Type $structs}}){{end}}

			return *outstruct, err
			{{else}}
			if err != nil {
				return {{range $i, $_ := .Normalized.Outputs}}*new({{bindtype .Type $structs}}), {{end}} err
			}
			{{range $i, $t := .Normalized.Outputs}}
			out{{$i}} := *abi.ConvertType(out[{{$i}}], new({{bindtype .Type $structs}})).(*{{bindtype .Type $structs}}){{end}}
			
			return {{range $i, $t := .Normalized.Outputs}}out{{$i}}, {{end}} err
			{{end}}
		}

With this contract it produces invalid code:

contract Eventer {
   
    event TestInt8(int8 indexed out1, int8 indexed out2);
    event AnonEvent(address, address);
    
    function getEvent() public {
        // set to 2,3 for functioning filter
        emit TestInt8(-2, -3);
    }

    function anonEvent() public {
        emit AnonEvent(msg.sender, msg.sender);
    }

    function fail() public {
        require(false, "error string");
    }
}

Produced code:

func (_Eventer *Eventer) UnpackGetEvent(data []byte) (struct{}, error) {
	out, err := _Eventer.abi.Unpack("getEvent", data)

	outstruct := new(struct{})
	if err != nil {
		return *outstruct, err
	}

	return *outstruct, err

}

(out is never used)

I think the issue is that you generate Unpacking functions for solidity functions that have no return value. I think those functions can just be skipped

[MariusVanDerWijden]

/home/matematik/go/src/github.com/go-snippets/geth-test/array.go:8:2: "fmt" imported and not used

You should probably do with fmt the same thing as with the other imports: _ = fmt.Printf so that go doesn't break if fmt is not used

[s1na]

Something is broken here:

Aha, we don't need an unpack method when there are no return params.

[geoknee]

@s1na are you still actively working on this PR? It is of interest to our project so we would be willing to devote some time to helping getting it over the line.

Our use case is: we are building a state channel client in Go. Currently, it manages a private key, and acts as a signer. It uses the abigen output to sign and launch transactions. What we want is to optionally have our client not manage a private key, and instead simply prepare (or "pack") the transaction and give it to the user to sign and send. Since we would like to maintain the coupling between or .sol source files are our go code, a neat way to achieve this is some changes to abigen such as those proposed on this PR.

One suggestion I would have is to maintain backward compatibility with the current version of abigen by simply adding the extra PackFoo (etc) functions to the existing output? Then you wouldn't need to worry about putting it all under a "v2" namespace or even adding those Transact helper functions...

[s1na]

Hi @geoknee, great to see your interest in this PR. I'm not actively working on this, although I plan to finish it at some point. Your help is appreciated.

I want to first address

One suggestion I would have is to maintain backward compatibility with the current version of abigen by simply adding the extra PackFoo (etc) functions to the existing output? Then you wouldn't need to worry about putting it all under a "v2" namespace or even adding those Transact helper functions...

We really want to get v2 out instead of packing more features into v1.

I saw you have already generated v2 bindings for your contract. It'd be already great to hear if things are working and you've had any friction points.

The biggest outstanding point is to add a test suite for v2.

[geoknee]

We really want to get v2 out instead of packing more features into v1.

Fair enough, we can use both side by side in our project without too much pain.

I saw you have already generated v2 bindings for your contract. It'd be already great to hear if things are working and you've had any friction points.

I did spot a couple of problems:

1. One which I suspect is a bug -- some unexpected db field which looks like it could be from your example contract sneaking into our bindings:

https://github.com/statechannels/go-nitro/commit/a1e9dd7466ebd262cea415db2c1e1ba2204bee22

I pushed a fix here https://github.com/s1na/go-ethereum/pull/10.

2. I get an error when trying to generate bindings for an ERC20 Token that is part of our setup. This has also been thrown up in some tests I started to sketch out https://github.com/s1na/go-ethereum/pull/11/files#r1269362198

The biggest outstanding point is to add a test suite for v2.

~How should this look like? Is there an existing test for v1 which we can emulate?~ I started hacking with the existing tests to extend their coverage. Keen to get feedback on that approach before devoting the time to finishing that off.

[jwasinger]

@MariusVanDerWijden I have a few things I am trying to wrap with this. It's in a bit of a messy state, and I'd ask to hold off on review until I can get this finished (couple of hours).

[jwasinger]

Ah, so I need to support custom errors with this too.

[jwasinger]

I noticed for deployments that I share one TransactOpts instance for many deployments. It only works because the nonce used in test cases is nil (so it gets filled automatically at deploy time).

[jwasinger]

Also, I'm curious - - why do we have .sol-files? Is it fi we want to update the abi-json at some later point? Or do we actually have a solc compilation step somewhere in our pipeline?

We don't have a compilation step in the pipeline. Unfortunately, this means that there's no way to determine whether a combine-abi.json and it's corresponding .sol are up-to-date as part of the CI. However, I think that adding solc binary requirement into our CI just for this seems like overkill?

I'd opt to keep the contract source for the ability to update down-the-line. However, we could just as well omit them as long as the purpose for each included ABI is documented.

[holiman]

I'd opt to keep the contract source for the ability to update down-the-line.

Yeah that's what I thought -- and I agree. I was just wondering if my thought was correct.

[MariusVanDerWijden]

Looks like the tests fail at the moment @jwasinger because the variable Str is being redeclared multiple times. I will take a deeper look once thats fixed. One note that I have already, do we need to check in the autogenerated v2 stubs, or is it possible to generate them on the fly during testing?

[jwasinger]

One note that I have already, do we need to check in the autogenerated v2 stubs, or is it possible to generate them on the fly during testing?

I'd lean towards not generating them on the fly. We've discussed this previously and concluded that generating them at runtime makes the testing logic more complicated than it has to be.

We test the properties of the generated bindings in various testcases in accounts/abi/bind/v2/lib_test.go and we also check to ensure that regenerating the bindings doesn't cause any mutations (in TestBindingGeneration). So this seems sufficient IMO (and probably a bit more comprehensive than on-the-fly generation).

[fjl]

I've spent quite a bit of time with this today, and can say it's not fully ready.

• We need to ensure users don't have to import both v1 and v2 in order to use v2.
  • We can consider moving all the auth.go, base.go, util.go code into v2, then create forwarding declarations in v1.
  • The new deployment logic should probably be moved into v2.
• New deployment logic needs more documentation.
• Generated types, Pack and Unpack functions need better documentation comments.
• For events, we should probably generate a pack function that creates topics.

[fjl]

Looks like there is a code issue in the deploy tree implementation. bind.MetaData cannot be copied because it contains a sync.Mutex value. The warning was silenced using a nolint:all comment. This is not OK.

[jwasinger]

Looks like there is a code issue in the deploy tree implementation. bind.MetaData cannot be copied because it contains a sync.Mutex value. The warning was silenced using a nolint:all comment. This is not OK.

@fjl This is only done in the test coverage for testing LinkDeploy, and it shouldn't matter there because we are not ever accessing those mutexes concurrently in test coverage.

[jwasinger]

I can create a deep-copy utility function to remove the lint exceptions though.

[jwasinger]

We should add a test case that generates bindings from an abi (not combined-json, so excluding the contract bytecode). Then instantiate a BoundContract from an already-deployed instance of the contract and interact with it.

[jwasinger]

CI failing BindingV2ConvertedV1Tests but I cannot reproduce it locally (even after cloning a fresh copy of the branch). I'm not sure how to restart a build on appveyor without pushing another commit (I assume the failing CI is just some fluke).

[jwasinger]

One thing that's kind of funky: LinkAndDeploy returns a DeploymentResult instance which contains a map of deployment addresses and transactions keyed by solidity library link pattern. If we create the bindings for a single contract with --abi, the MetaData object in the produced bindings doesn't set the Pattern field: the link pattern is not defined given the ABI definition alone.

In general, it's weird that the user has to use the library link pattern to identify which txs/addresses correspond to which contracts in the DeploymentResult. I think it's worth adding an additional field (FullName) to the MetaData which in the case of --combined-json is the fully-qualified contract name (relative/path/from/toplevel/project/dir/to/contract.sol, the source string used to calculate the library link pattern). In the case of building bindings for a single contract with --abi, this will just be the contract type name.

[jwasinger]

The current CI failures appear to be from some non-deterministic behavior in the binding... I will look into it further tomorrow.

I pushed some commits which attempt to clean up the contract interaction API. I also drafted a commit which adds a default DeployFn implementation option for the deployer, but it's not quite finished.

[jwasinger]

The commit before 6c70418313b56f96522d2dcf56f21931ce71a13d is not really complete/correct and should be squashed. but I'm not going to force-push it out atm.

[jwasinger]

Off the top of my head, I think this PR is mostly in a good spot. I've changed the API through which we interact with contracts fairly significantly: with v2, the BoundContract doesn't expose any public methods itself but is used by the v2 utility methods. I felt that previously, it was weird to have a BoundContract that has member methods used by the v1 bindings, while we also were intending it to be used with the V2 methods: the functionality was overlapping and the overall API was inconsistent. Note that these changes to BoundContract don't change the interface from the v1 perspective.

My priorities on this project for today are:

• review and overhaul the code documentation where appropriate.
• finish the draft of the abigen v2 website page.

[rjl493456442]

The fallback and receive function are not rendered in the binding, which I think should be useful.

A contract can have at most one fallback function, declared using either fallback () external [payable] or fallback (bytes calldata input) external [payable] returns (bytes memory output)

Especially for fallback, it can support bytes input. It would be very useful to have the fallback encoder and combine it with other function encoder output.

[jwasinger]

Especially for fallback, it can support bytes input. It would be very useful to have the fallback encoder and combine it with other function encoder output.

So in this case, the input to fallback is not abi-encoded, so generating an encoder in the bindings is not strictly required.

That being said, I'm fine with generating packing functions when fallback/receive are defined (and an unpack if fallback is of the signature (bytes calldata input) external [payable] returns (bytes memory output), which is the only form allowed other than () external [payable]). It makes the code for interacting with these methods more consistent with how it is done with other custom named methods.

[jwasinger]

I found out that if a fallback is specified, its signature is not specified in the ABI definition (with the exception of whether it is payable or not).

Even if this could be fixed in solidity, we still have to deal with the current behavior.

Accordingly, I think the best option for us will be to provide generic pack/unpack for fallback from lib.go. These will take/return []byte without doing any encoding/decoding. Then we note in the docs that interaction via the fallback can optionally use these based on which variant of the fallback was defined (noting the lack of distinction in the ABI definition).

[wisehalvdan]

Is this ready to be merged ? or will it be merged any time soon ?

[fjl]

@wisehalvdan soon!

[jwasinger]

Before we merge this, I want to reopen so that I get main authorship of the PR.