ABIGEN v2 #26782

s1na · 2023-02-28T16:03:38Z

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.

accounts/abi/bind/bind.go

accounts/abi/bind/lib.go

MariusVanDerWijden

Looks pretty cool! I don't like the *2 naming, but I guess Felix likes it, so I won't argue to hard against it. I'm going to do some more tests and then approve

accounts/abi/bind/lib.go

MariusVanDerWijden · 2023-03-01T09:15:35Z

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 · 2023-03-01T09:23:11Z

/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

accounts/abi/bind/template2.go

s1na · 2023-03-01T09:47:33Z

Something is broken here:

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

fjl

Please add some tests, and more documentation.

geoknee · 2023-07-19T14:14:35Z

@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 · 2023-07-19T14:26:15Z

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 · 2023-07-20T08:53:30Z

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:

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:

statechannels/go-nitro@a1e9dd7

I pushed a fix here s1na#10.

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.

accounts/abi/bind/v2/v2_test.go

MariusVanDerWijden · 2024-11-26T06:22:29Z

accounts/abi/bind/v2/v2_test.go

+	"context"
+	"encoding/json"
+	"github.com/ethereum/go-ethereum/accounts/abi/bind/backends"
+	"github.com/ethereum/go-ethereum/accounts/abi/bind/testdata/v2_generated_testcase"


Build fails on this import atm

jwasinger · 2024-11-26T06:25:27Z

@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).

accounts/abi/bind/v2/backend.go

holiman · 2024-12-18T09:40:12Z

accounts/abi/bind/dep_tree.go

+		err := deployer.linkAndDeploy(tr)
+		res := deployer.result()
+		accumRes.Accumulate(res)
+		if err != nil {
+			return accumRes, err
+		}


Suggested change

err := deployer.linkAndDeploy(tr)

res := deployer.result()

accumRes.Accumulate(res)

if err != nil {

return accumRes, err

}

if err := deployer.linkAndDeploy(tr); err != nil {

return accumRes, err

}

accumRes.Accumulate(deployer.result())

Afaict, if it linkAndDeploy exits with errors, there's nothing new to add to the accumulated result, so might aswell write it a bit more go-idiomatic

If the call to linkAndDeploy deploys multiple contracts and then fails at some point, we want the accumulated result to include whatever succeeded.

Right. I didn't see that there's a recursive linkAndDeploy internally, which might add things and still return with error later.

holiman · 2024-12-18T09:41:53Z

accounts/abi/bind/dep_tree.go

+		Addrs: make(map[string]common.Address),
+	}
+	for _, meta := range deployParams.contracts {
+		unlinkedContracts[meta.Pattern] = meta.Bin[2:]


Are we fully in control of the inputs here? Do we know with 100% certainty that the meta.Bin starts with 0x-prefix?

I assume that users are sourcing everything from the output of solidity abi compilation (and it does prefix the contract bin with 0x). That's why I haven't yet added more defensive checks throughout this PR.

I don't think the contract deployer can be empty in the ABI. I can see no reason why that would ever be a valid result of the solidity compilation process.

holiman · 2024-12-18T09:48:57Z

accounts/abi/bind/dep_tree.go

+	for _, tr := range deps {
+		deployer := newDepTreeDeployer(deploy)
+		if deployParams.inputs != nil {
+			deployer.input = map[string][]byte{tr.pattern: deployParams.inputs[tr.pattern]}


Why is it named tr? It's definitely not obvious to a casual reader.
And what does pattern signify, really? Please document or name it more appropriate. For me, it's a bit black magic what is happening here, with the setting of deployer.input to a map, involving the pattern, if the deployParams.inputs is non-nil.

tr represents the root of a deployment tree here: it's a contract/library that isn't a dependency of any other contracts.

a contract's pattern is a legitimate solidity concept (reference): it's the "34 character prefix of the hex encoding of the keccak256 hash of the fully qualified library name". A unique identifier for each contract/library.

I think for clarity, it probably deserves its own type (even if just a typedef on string).

…ndency tree handling as a result of embedding it in the bindings.

holiman · 2024-12-19T07:42:06Z

accounts/abi/bind/dep_tree.go

+	overrideAddrs map[string]common.Address
+	deployedAddrs map[string]common.Address


I think you can drop overrideAddrs, and shove everything into deployedAddrs. The deployerTxs can be used if the user wants to know which happened now and which happened previously

WDYT about filtering the overrides out of the DeploymentResult at the end of LinkAndDeploy based on their non-presence in the map of deployed txs?

Something like that, yes.
Here are some suggested changes: holiman@4ad21ba
I wanted to try it out in code to see if it worked. Seems to, so far

^ as you can see in that commit, I don't see any need for Accumulate to exist? It's accumulated in the deployer

holiman · 2024-12-19T07:57:46Z

accounts/abi/bind/dep_tree_test.go

+	return rootMetadatas
+}
+
+func __linkDeps(metadata MetaData, depMap map[string]*MetaData, roots *map[string]struct{}) MetaData {


What's up with this name? Why such a strange prefix?

that's the recursing part of linkDeps. I didn't know what to call it. That code needs a bit of love (documentation/renaming where appropriate) but I'm trying to port the v1 bind tests over to v2 atm.

Well, that's now how we name things in go (the underscores). I'm sure you can figure out a more descriptive name

accounts/abi/bind/dep_tree_test.go

holiman · 2024-12-19T08:33:10Z

accounts/abi/bind/dep_tree.go

+	// if this contract/library depends on other libraries deploy them (and their dependencies) first
+	for _, dep := range metadata.Deps {
+		if err := d.linkAndDeploy(dep); err != nil {
+			return err
+		}
+	}
+	// if we just deployed any prerequisite contracts, link their deployed addresses into the bytecode to produce
+	// a deployer bytecode for this contract.
+	deployerCode := metadata.Bin
+	for _, dep := range metadata.Deps {
+		linkAddr, ok := d.deployedAddrs[dep.Pattern]
+		if !ok {
+			linkAddr = d.overrideAddrs[dep.Pattern]
+		}
+		deployerCode = strings.ReplaceAll(deployerCode, "__$"+dep.Pattern+"$__", strings.ToLower(linkAddr.String()[2:]))
+	}


You can combine these two steps, and also make it look nicer if you change the signature to return common.Address, error:

// if this contract/library depends on other libraries deploy them (and their dependencies) first deployerCode := metadata.Bin for _, dep := range metadata.Deps { addr, err := d.linkAndDeploy(dep) if err != nil { return common.Address{}, err } // link their deployed addresses into the bytecode to produce deployerCode = strings.ReplaceAll(deployerCode, "__$"+dep.Pattern+"$__", strings.ToLower(addr.String()[2:])) }

(still store the addr/tx as previous, though)

See second commit in 62a7806

Cool. I'm going to pull these in to the branch.

BTW, I wouldn't mind if you squash things a bit, perhaps squash sequences of commits from one author into one commit.

holiman · 2024-12-19T08:50:30Z

accounts/abi/bind/dep_tree.go

+		if deployParams.inputs != nil {
+			deployer.input = map[string][]byte{contract.Pattern: deployParams.inputs[contract.Pattern]}
+		}


This clause is not covered by testing (commenting it out doesn't cause failures). And it's not surprising, because the clause doesn't make sense.
On each iteration of a contract, it overwrites the input-map with the current contract input.
Why not just copy/clone the deployParams.inputs map to the deployer, already when creating the deployer?

Afaict you can turn it into

func LinkAndDeploy(deployParams *DeploymentParams, deploy DeployFn) (res *DeploymentResult, err error) { deployer := newDepTreeDeployer(deployParams.overrides, deploy, deployParams.inputs) for _, contract := range deployParams.contracts { if _ err := deployer.linkAndDeploy(contract); err != nil { return deployer.result(), err } } return deployer.result(), nil

holiman · 2024-12-19T09:13:14Z

accounts/abi/bind/bindv2.go

+func (b *contractBinder) RegisterCallIdentifier(id string) (string, error) {
+	return b.registerIdentifier(b.callIdentifiers, id)
+}


WHy is this public accessor needed? Are users expected to use this? In what scenario?

it's a public method on a private type. Will move it to lowercase.

it's a public method on a private type. Will move it to lowercase.

i.e. I didn't mean it for public consumption. a mistake on my part.

Well, if it's not for public, is it even needed? doesn't save a whole lot of typing really

not really needed no.

holiman · 2024-12-19T09:16:02Z

accounts/abi/bind/bind.go

-	LangGo: abi.ToCamelCase,
-}
+// conform to Go naming conventions.
+var methodNormalizer = abi.ToCamelCase


Why is this needed at all (now that you removed the lang-dependent map). It's never actually changed, is it? So why not just use abi.ToCamelCase and keep it simple?

Same with capitalise below, really

holiman · 2024-12-19T09:32:33Z

accounts/abi/bind/bindv2.go

+	normalized.Inputs = make([]abi.Argument, len(original.Inputs))
+	copy(normalized.Inputs, original.Inputs)


Suggested change

normalized.Inputs = make([]abi.Argument, len(original.Inputs))

copy(normalized.Inputs, original.Inputs)

normalized.Inputs = slices.Clone(original.Inputs)

And same for outputs.

…e subtest that failed.

holiman · 2024-12-19T10:00:42Z

accounts/abi/bind/bindv2.go

+
+// normalizeErrorOrEventFields normalizes errors/events for emitting through bindings:
+// Any anonymous fields are given generated names.
+func (cb *contractBinder) normalizeErrorOrEventFields(originalInputs abi.Arguments) abi.Arguments {


This method does multiple things. Howver, it only uses cb in order to do this:

if hasStruct(input.Type) { cb.binder.BindStructType(input.Type) }

Hence, would it be possible to change into

// normalizeErrorOrEventFields normalizes errors/events for emitting through bindings: // Any anonymous fields are given generated names. func (cb *contractBinder) normalizeErrorOrEventFields(originalInputs abi.Arguments) abi.Arguments { args := normalizeArgs(originalInputs) // Soup up the struct types for _, input := range args { if hasStruct(input.Type) { cb.binder.BindStructType(input.Type) } } return args } func normalizeArgs(originalInputs abi.Arguments) abi.Arguments { ...

?
Would make it more testable

accounts/abi/bind/dep_tree_test.go

…vate variable

holiman · 2024-12-19T10:17:18Z

accounts/abi/bind/bindv2.go

+			if !used[capitalise(normalizedArguments[i].Name)] {
+				used[capitalise(normalizedArguments[i].Name)] = true
+				break
+			}
+			normalizedArguments[i].Name = fmt.Sprintf("%s%d", normalizedArguments[i].Name, index)


this looks slightly wrong to me. We check for capitalized version of the name, and then iterate on changing the name. But we don't actually capitalize the name. So the used-check uses the capitalized version, but non-capitalized is set as name.

It's a big confusing, and looks like a bug

Shouldn't it be something like this?

func normalizeArguments(args abi.Arguments) abi.Arguments { args := slices.Clone(args) used := make(map[string]bool) for i, input := range args { if input.Name == "" || isKeyWord(input.Name) { args[i].Name = fmt.Sprintf("arg%d", i) } name := capitalise(args[i].Name) for index := 0; ; index++ { if !used[name] { used[name] = true break } // try next name = capitalise(fmt.Sprintf("%s%d", args[i].Name, index)) } args[i].Name = name } return args }

holiman · 2024-12-19T12:09:41Z

accounts/abi/bind/bindv2.go

+}
+
+// bindEvent normalizes an event and registers it to be emitted in the bindings.
+func (cb *contractBinder) bindEvent(original abi.Event) error {


Please move bindEvent and bindError so they are right below bindMethod

accounts/abi/bind/bindv2.go

accounts/abi/bind/template.go

jwasinger · 2024-12-19T12:46:17Z

accounts/abi/bind/template.go

+type tmplDataV2 struct {
+	Package   string                     // Name of the package to place the generated file in
+	Contracts map[string]*tmplContractV2 // List of contracts to generate into this file
+	Libraries map[string]string          // Map of the contract's name to link pattern


This is the only thing that's different which warranted addition of templDataV2.

…t fails before the fix, and also also a few more test cases for args normalization.

…e registration

…t the backing-array provided by the API user from being mutated). re-enable deployment-with-overrides test

…the results of feeding v1 binding test ABIs through the v2 generator.

s1na requested review from gballet and MariusVanDerWijden as code owners February 28, 2023 16:03

s1na changed the title ~~cmd,accounts/abi: abigen v2~~ cmd, accounts/abi: abigen v2 Feb 28, 2023

MariusVanDerWijden reviewed Mar 1, 2023

View reviewed changes

accounts/abi/bind/bind.go Outdated Show resolved Hide resolved

s1na force-pushed the abigen2 branch from 5a3fb11 to 3d0330d Compare March 1, 2023 08:45

MariusVanDerWijden reviewed Mar 1, 2023

View reviewed changes

accounts/abi/bind/lib.go Outdated Show resolved Hide resolved

MariusVanDerWijden reviewed Mar 1, 2023

View reviewed changes

accounts/abi/bind/lib.go Outdated Show resolved Hide resolved

accounts/abi/bind/lib.go Outdated Show resolved Hide resolved

MariusVanDerWijden reviewed Mar 1, 2023

View reviewed changes

accounts/abi/bind/template2.go Outdated Show resolved Hide resolved

MariusVanDerWijden reviewed Mar 1, 2023

View reviewed changes

accounts/abi/bind/template2.go Outdated Show resolved Hide resolved

fjl mentioned this pull request Mar 7, 2023

[proposal] abigen: add encoder to pack or unpack functions #26360

Closed

s1na mentioned this pull request Mar 7, 2023

is there a way to callStatic func like in ethers js? #26254

Open

fjl requested changes Apr 18, 2023

View reviewed changes

fjl changed the title ~~cmd, accounts/abi: abigen v2~~ ABIGEN v2 Apr 18, 2023

s1na mentioned this pull request May 26, 2023

feature(abigen): support parse transaction input bytes readable go struct in abigen #27361

Closed

geoknee mentioned this pull request Jul 18, 2023

Allow for the consumer to sign their own chain transactions statechannels/go-nitro#1416

Open

fjl mentioned this pull request Feb 14, 2024

ethereum, ethclient: add blob transaction fields in CallMsg #28989

Merged

This was referenced Oct 29, 2024

It would be helpful if abigen provided an easy way to manually call estimateGas #29798

Open

ABIGEN v2 tracking issue #30701

Open

jwasinger force-pushed the abigen2 branch 2 times, most recently from c044348 to 008028e Compare November 4, 2024 10:32

MariusVanDerWijden reviewed Nov 26, 2024

View reviewed changes

accounts/abi/bind/v2/v2_test.go Outdated Show resolved Hide resolved

MariusVanDerWijden reviewed Nov 26, 2024

View reviewed changes

accounts/abi/bind/v2/backend.go Outdated Show resolved Hide resolved

holiman reviewed Dec 18, 2024

View reviewed changes

jwasinger added 2 commits December 18, 2024 21:07

wip: embed library relations in metadata

6d626c4

embed direct library relations in metadata in bindings. simplify depe…

0d443b8

…ndency tree handling as a result of embedding it in the bindings.