accounts/abi: resolve name conflict for methods starting with a number #26999

MariusVanDerWijden · 2023-03-28T10:09:11Z

Its a very edge case, but this PR allows abigen to generate bindings for methods starting with a number
closes #26972

holiman · 2023-03-28T10:44:36Z

// If a method name starts with a number an m is prepended (e.g. 1method -> m1method).

Isn't this just pushing the edgecase a bit further away? What if we already have a method called m1method and 1method ?

EDIT: Ah then we'd get m1method and m1method2.

MariusVanDerWijden · 2023-03-28T10:48:06Z

The edge case is variables starting with a number, this will crash the abigen. This PR fixes that, so a contract with m1method 1method 1method would create m1method m1method1 m1method2`. So we resolve both the naming conflict and the variable starting with number conflict

holiman · 2023-03-28T10:48:14Z

accounts/abi/utils.go

 func ResolveNameConflict(rawName string, used func(string) bool) string {
 	name := rawName
+	if unicode.IsDigit(rune(name[0])) {
+		name = fmt.Sprintf("%s%s", "m", name)
+	}
 	ok := used(name)
 	for idx := 0; ok; idx++ {
 		name = fmt.Sprintf("%s%d", rawName, idx)


This would also need to use the m-prefixed version. So maybe you need to set rawName to name

Damn now I see it, thanks will add a test

holiman · 2023-03-28T10:49:29Z

This PR fixes that, so a contract with m1method 1method 1method would create

If you had written a test you'd know that wasn't true :p

holiman

LGTM!

holiman · 2023-03-29T09:24:18Z

--- FAIL: TestReader (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0
goroutine 19 [running]:
testing.tRunner.func1.2({0x76e260, 0xc0000c86f0})
	/home/appveyor/.cache/geth-go-1.20.2-linux-amd64/go/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/home/appveyor/.cache/geth-go-1.20.2-linux-amd64/go/src/testing/testing.go:1529 +0x39f
panic({0x76e260, 0xc0000c86f0})
	/home/appveyor/.cache/geth-go-1.20.2-linux-amd64/go/src/runtime/panic.go:884 +0x213
github.com/ethereum/go-ethereum/accounts/abi.ResolveNameConflict({0x0?, 0xea8?}, 0x1dff?)
	/home/appveyor/projects/go-ethereum/accounts/abi/utils.go:38 +0x1c8
github.com/ethereum/go-ethereum/accounts/abi.(*ABI).UnmarshalJSON(0xc0000e0900, {0xc00012e001, 0xea8, 0x1dff})
	/home/appveyor/projects/go-ethereum/accounts/abi/abi.go:167 +0x73b
encoding/json.(*decodeState).array(0xc0000d8a28, {0x76e8a0?, 0xc0000e0900?, 0xc0000d5240?})

Oops?

accounts/abi/utils.go

Co-authored-by: Martin Holst Swende <martin@swende.se>

holiman · 2023-03-30T19:08:12Z

--- FAIL: TestCrashers (0.00s)
panic: reflect.StructOf: field "m1" is unexported but missing PkgPath [recovered]
	panic: reflect.StructOf: field "m1" is unexported but missing PkgPath
goroutine 49 [running]:
testing.tRunner.func1.2({0xe2ec80, 0xc00039e910})
	/home/appveyor/.cache/geth-go-1.20.2-linux-amd64/go/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/home/appveyor/.cache/geth-go-1.20.2-linux-amd64/go/src/testing/testing.go:1529 +0x39f
panic({0xe2ec80, 0xc00039e910})
	/home/appveyor/.cache/geth-go-1.20.2-linux-amd64/go/src/runtime/panic.go:884 +0x213

s1na

LGTM

s1na · 2023-04-05T12:12:20Z

Wait, I'm confused:

❯ solc -o build --bin --ast-compact-json --asm --abi --overwrite test.sol
Error: Expected identifier but got 'ILLEGAL'
  --> test.sol:42:14:
   |
42 |     function 1get(uint k) public returns (uint) {
   |              ^

s1na · 2023-04-07T12:39:55Z

Aha so as @jsvisa mentioned, the problematic pattern is first _ then digit. I wrote a test which should catch it:

diff --git a/accounts/abi/bind/bind_test.go b/accounts/abi/bind/bind_test.go
index cbbce7b30..c16f0c752 100644
--- a/accounts/abi/bind/bind_test.go
+++ b/accounts/abi/bind/bind_test.go
@@ -2038,6 +2038,23 @@ var bindTests = []struct {
 				t.Errorf("error deploying the contract: %v", err)
 			}
 		`,
+	}, {
+		name: "NumericMethodName",
+		contract: `
+		// SPDX-License-Identifier: GPL-3.0
+		pragma solidity >=0.4.22 <0.9.0;
+		contract NumericMethodName {
+			function _1test() public pure {}
+		}
+		`,
+		bytecode: []string{"0x6080604052348015600f57600080fd5b50606d80601d6000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c8063ffa0279514602d575b600080fd5b60336035565b005b56fea2646970667358221220b37b7ec9ac04fb43318f8e3c53ce78e5f4477258cf28f81181359bf8be73c75264736f6c63430008110033"},
+		abi:      []string{`[{"inputs":[],"name":"_1test","outputs":[],"stateMutability":"pure","type":"function"}]`},
+		imports:  ``,
+		tester: `
+			if b, err := NewNumericMethodName(common.Address{}, nil); b == nil || err != nil {
+				t.Fatalf("combined binding (%v) nil or error (%v) not nil", b, nil)
+			}
+`,
 	},
 }

jsvisa · 2023-04-10T04:54:51Z

Seems we need to convert field.Name into CamelCase before ResolveNameConflict? eg:

diff --git a/accounts/abi/abi.go b/accounts/abi/abi.go
index 62b860e18..7a03209f2 100644
--- a/accounts/abi/abi.go
+++ b/accounts/abi/abi.go
@@ -164,7 +164,8 @@ func (abi *ABI) UnmarshalJSON(data []byte) error {
                case "constructor":
                        abi.Constructor = NewMethod("", "", Constructor, field.StateMutability, field.Constant, field.
                case "function":
-                       name := ResolveNameConflict(field.Name, func(s string) bool { _, ok := abi.Methods[s]; return
+                       name := ToCamelCase(field.Name)
+                       name = ResolveNameConflict(name, func(s string) bool { _, ok := abi.Methods[s]; return ok })
                        abi.Methods[name] = NewMethod(name, field.Name, Function, field.StateMutability, field.Constan
                case "fallback":
                        // New introduced function type in v0.6.0, check more detail
@@ -184,7 +185,8 @@ func (abi *ABI) UnmarshalJSON(data []byte) error {
                        }
                        abi.Receive = NewMethod("", "", Receive, field.StateMutability, field.Constant, field.Payable,
                case "event":
-                       name := ResolveNameConflict(field.Name, func(s string) bool { _, ok := abi.Events[s]; return o
+                       name := ToCamelCase(field.Name)
+                       name = ResolveNameConflict(name, func(s string) bool { _, ok := abi.Events[s]; return ok })
                        abi.Events[name] = NewEvent(name, field.Name, field.Anonymous, field.Inputs)
                case "error":
                        // Errors cannot be overloaded or overridden but are inherited,

s1na · 2023-04-24T13:23:20Z

I pushed a commit to rather than names starting with digits catch names starting with _%d. Also I've done it during the binding generation. I felt the issue is specifically related to generating Go bindings that crash with leading digit names, and not directly a problem for other ABI operations.

MariusVanDerWijden

LGTM, I've removed the now obsolete test

ethereum#26999) This adds logic to prepend 'M' or 'E' to Solidity identifiers when they would otherwise violate Go identifier naming rules. Closes ethereum#26972 --------- Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>

… a number (ethereum#26999)" This reverts commit 5b6fa97.

accounts/abi: resolve name conflict for methods starting with a number

5d5f243

MariusVanDerWijden requested a review from gballet as a code owner March 28, 2023 10:09

accounts/abi: strings > values

9f175c6

holiman reviewed Mar 28, 2023

View reviewed changes

accounts/abi: fix conflict resolution, add tests

7da4285

holiman approved these changes Mar 28, 2023

View reviewed changes

rjl493456442 approved these changes Mar 28, 2023

View reviewed changes

holiman reviewed Mar 29, 2023

View reviewed changes

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

MariusVanDerWijden and others added 2 commits March 29, 2023 11:31

Update accounts/abi/utils.go

9f91e00

Co-authored-by: Martin Holst Swende <martin@swende.se>

accounts/abi: add test

8507eca

accounts/abi: capitalize prefix

a5dc1ea

s1na approved these changes Apr 5, 2023

View reviewed changes

Check for names prefixed with _

a2f34b8

s1na and others added 2 commits April 26, 2023 11:27

Merge branch 'master' into numeric-abi-methods

14e17a0

accounts/abi: reduce diff

cdd46fb

MariusVanDerWijden commented May 2, 2023

View reviewed changes

fjl merged commit ac3418d into ethereum:master May 2, 2023

fjl added this to the 1.11.7 milestone May 2, 2023

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "accounts/abi: resolve name conflict for methods starting with…

2094bee

… a number (ethereum#26999)" This reverts commit 5b6fa97.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "accounts/abi: resolve name conflict for methods starting with…

d3c7707

… a number (ethereum#26999)" This reverts commit 5b6fa97.

ian0371 mentioned this pull request Dec 31, 2024

abi/bind: Fix bugs kaiachain/kaia#195

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

accounts/abi: resolve name conflict for methods starting with a number #26999

accounts/abi: resolve name conflict for methods starting with a number #26999

MariusVanDerWijden commented Mar 28, 2023

holiman commented Mar 28, 2023 •

edited

Loading

MariusVanDerWijden commented Mar 28, 2023

holiman Mar 28, 2023

MariusVanDerWijden Mar 28, 2023 •

edited

Loading

holiman commented Mar 28, 2023

holiman left a comment

holiman commented Mar 29, 2023

holiman commented Mar 30, 2023

s1na left a comment

s1na commented Apr 5, 2023

s1na commented Apr 7, 2023 •

edited

Loading

jsvisa commented Apr 10, 2023

s1na commented Apr 24, 2023 •

edited

Loading

MariusVanDerWijden left a comment

accounts/abi: resolve name conflict for methods starting with a number #26999

accounts/abi: resolve name conflict for methods starting with a number #26999

Conversation

MariusVanDerWijden commented Mar 28, 2023

holiman commented Mar 28, 2023 • edited Loading

MariusVanDerWijden commented Mar 28, 2023

holiman Mar 28, 2023

Choose a reason for hiding this comment

MariusVanDerWijden Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

holiman commented Mar 28, 2023

holiman left a comment

Choose a reason for hiding this comment

holiman commented Mar 29, 2023

holiman commented Mar 30, 2023

s1na left a comment

Choose a reason for hiding this comment

s1na commented Apr 5, 2023

s1na commented Apr 7, 2023 • edited Loading

jsvisa commented Apr 10, 2023

s1na commented Apr 24, 2023 • edited Loading

MariusVanDerWijden left a comment

Choose a reason for hiding this comment

holiman commented Mar 28, 2023 •

edited

Loading

MariusVanDerWijden Mar 28, 2023 •

edited

Loading

s1na commented Apr 7, 2023 •

edited

Loading

s1na commented Apr 24, 2023 •

edited

Loading