Skip to content

Commit

Permalink
accounts/abi: Prevent recalculation of internal fields (ethereum#20895)
Browse files Browse the repository at this point in the history 
* accounts/abi: prevent recalculation of ID, Sig and String

* accounts/abi: fixed unpacking of no values

* accounts/abi: multiple fixes to arguments

* accounts/abi: refactored methodName and eventName

This commit moves the complicated logic of how we assign method names
and event names if they already exist into their own functions for
better readability.

* accounts/abi: prevent recalculation of internal

In this commit, I changed the way we calculate the string
representations, sig representations and the id's of methods. Before
that these fields would be recalculated everytime someone called .Sig()
.String() or .ID() on a method or an event.

Additionally this commit fixes issue ethereum#20856 as we assign names to inputs
with no name (input with name "" becomes "arg0")

* accounts/abi: added unnamed event params test

* accounts/abi: fixed rebasing errors in method sig

* accounts/abi: fixed rebasing errors in method sig

* accounts/abi: addressed comments

* accounts/abi: added FunctionType enumeration

* accounts/abi/bind: added test for unnamed arguments

* accounts/abi: improved readability in NewMethod, nitpicks

* accounts/abi: method/eventName -> overloadedMethodName
  • Loading branch information
@MariusVanDerWijden @abramsymons
MariusVanDerWijden authored and abramsymons committed May 6, 2020
1 parent f33f57b commit fef9d4e
Show file tree
Hide file tree
Showing 14 changed files with 273 additions and 233 deletions.
124 changes: 44 additions & 80 deletions accounts/abi/abi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func (abi ABI) Pack(name string, args ...interface{}) ([]byte, error) {
return nil, err
}
// Pack up the method ID too if not a constructor and return
return append(method.ID(), arguments...), nil
return append(method.ID, arguments...), nil
}

// Unpack output in v according to the abi specification
Expand Down Expand Up @@ -139,59 +139,17 @@ func (abi *ABI) UnmarshalJSON(data []byte) error {
for _, field := range fields {
switch field.Type {
case "constructor":
abi.Constructor = Method{
Inputs: field.Inputs,

// Note for constructor the `StateMutability` can only
// be payable or nonpayable according to the output of
// compiler. So constant is always false.
StateMutability: field.StateMutability,

// Legacy fields, keep them for backward compatibility
Constant: field.Constant,
Payable: field.Payable,
}
abi.Constructor = NewMethod("", "", Constructor, field.StateMutability, field.Constant, field.Payable, field.Inputs, nil)
case "function":
name := field.Name
_, ok := abi.Methods[name]
for idx := 0; ok; idx++ {
name = fmt.Sprintf("%s%d", field.Name, idx)
_, ok = abi.Methods[name]
}
abi.Methods[name] = Method{
Name: name,
RawName: field.Name,
StateMutability: field.StateMutability,
Inputs: field.Inputs,
Outputs: field.Outputs,

// Legacy fields, keep them for backward compatibility
Constant: field.Constant,
Payable: field.Payable,
}
name := abi.overloadedMethodName(field.Name)
abi.Methods[name] = NewMethod(name, field.Name, Function, field.StateMutability, field.Constant, field.Payable, field.Inputs, field.Outputs)
case "fallback":
// New introduced function type in v0.6.0, check more detail
// here https://solidity.readthedocs.io/en/v0.6.0/contracts.html#fallback-function
if abi.HasFallback() {
return errors.New("only single fallback is allowed")
}
abi.Fallback = Method{
Name: "",
RawName: "",

// The `StateMutability` can only be payable or nonpayable,
// so the constant is always false.
StateMutability: field.StateMutability,
IsFallback: true,

// Fallback doesn't have any input or output
Inputs: nil,
Outputs: nil,

// Legacy fields, keep them for backward compatibility
Constant: field.Constant,
Payable: field.Payable,
}
abi.Fallback = NewMethod("", "", Fallback, field.StateMutability, field.Constant, field.Payable, nil, nil)
case "receive":
// New introduced function type in v0.6.0, check more detail
// here https://solidity.readthedocs.io/en/v0.6.0/contracts.html#fallback-function
Expand All @@ -201,49 +159,55 @@ func (abi *ABI) UnmarshalJSON(data []byte) error {
if field.StateMutability != "payable" {
return errors.New("the statemutability of receive can only be payable")
}
abi.Receive = Method{
Name: "",
RawName: "",

// The `StateMutability` can only be payable, so constant
// is always true while payable is always false.
StateMutability: field.StateMutability,
IsReceive: true,

// Receive doesn't have any input or output
Inputs: nil,
Outputs: nil,

// Legacy fields, keep them for backward compatibility
Constant: field.Constant,
Payable: field.Payable,
}
abi.Receive = NewMethod("", "", Receive, field.StateMutability, field.Constant, field.Payable, nil, nil)
case "event":
name := field.Name
_, ok := abi.Events[name]
for idx := 0; ok; idx++ {
name = fmt.Sprintf("%s%d", field.Name, idx)
_, ok = abi.Events[name]
}
abi.Events[name] = Event{
Name: name,
RawName: field.Name,
Anonymous: field.Anonymous,
Inputs: field.Inputs,
}
name := abi.overloadedEventName(field.Name)
abi.Events[name] = NewEvent(name, field.Name, field.Anonymous, field.Inputs)
default:
return fmt.Errorf("abi: could not recognize type %v of field %v", field.Type, field.Name)
}
}
return nil
}

// overloadedMethodName returns the next available name for a given function.
// Needed since solidity allows for function overload.
//
// e.g. if the abi contains Methods send, send1
// overloadedMethodName would return send2 for input send.
func (abi *ABI) overloadedMethodName(rawName string) string {
name := rawName
_, ok := abi.Methods[name]
for idx := 0; ok; idx++ {
name = fmt.Sprintf("%s%d", rawName, idx)
_, ok = abi.Methods[name]
}
return name
}

// overloadedEventName returns the next available name for a given event.
// Needed since solidity allows for event overload.
//
// e.g. if the abi contains events received, received1
// overloadedEventName would return received2 for input received.
func (abi *ABI) overloadedEventName(rawName string) string {
name := rawName
_, ok := abi.Events[name]
for idx := 0; ok; idx++ {
name = fmt.Sprintf("%s%d", rawName, idx)
_, ok = abi.Events[name]
}
return name
}

// MethodById looks up a method by the 4-byte id
// returns nil if none found
func (abi *ABI) MethodById(sigdata []byte) (*Method, error) {
if len(sigdata) < 4 {
return nil, fmt.Errorf("data too short (%d bytes) for abi method lookup", len(sigdata))
}
for _, method := range abi.Methods {
if bytes.Equal(method.ID(), sigdata[:4]) {
if bytes.Equal(method.ID, sigdata[:4]) {
return &method, nil
}
}
Expand All @@ -254,7 +218,7 @@ func (abi *ABI) MethodById(sigdata []byte) (*Method, error) {
// ABI and returns nil if none found.
func (abi *ABI) EventByID(topic common.Hash) (*Event, error) {
for _, event := range abi.Events {
if bytes.Equal(event.ID().Bytes(), topic.Bytes()) {
if bytes.Equal(event.ID.Bytes(), topic.Bytes()) {
return &event, nil
}
}
Expand All @@ -263,10 +227,10 @@ func (abi *ABI) EventByID(topic common.Hash) (*Event, error) {

// HasFallback returns an indicator whether a fallback function is included.
func (abi *ABI) HasFallback() bool {
return abi.Fallback.IsFallback
return abi.Fallback.Type == Fallback
}

// HasReceive returns an indicator whether a receive function is included.
func (abi *ABI) HasReceive() bool {
return abi.Receive.IsReceive
return abi.Receive.Type == Receive
}
77 changes: 48 additions & 29 deletions accounts/abi/abi_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,14 @@ const jsondata2 = `

func TestReader(t *testing.T) {
Uint256, _ := NewType("uint256", "", nil)
exp := ABI{
abi := ABI{
Methods: map[string]Method{
"balance": {
"balance", "balance", "view", false, false, false, false, nil, nil,
},
"send": {
"send", "send", "", false, false, false, false, []Argument{
{"amount", Uint256, false},
}, nil,
},
"balance": NewMethod("balance", "balance", Function, "view", false, false, nil, nil),
"send": NewMethod("send", "send", Function, "", false, false, []Argument{{"amount", Uint256, false}}, nil),
},
}

abi, err := JSON(strings.NewReader(jsondata))
exp, err := JSON(strings.NewReader(jsondata))
if err != nil {
t.Error(err)
}
Expand Down Expand Up @@ -173,22 +167,22 @@ func TestTestSlice(t *testing.T) {

func TestMethodSignature(t *testing.T) {
String, _ := NewType("string", "", nil)
m := Method{"foo", "foo", "", false, false, false, false, []Argument{{"bar", String, false}, {"baz", String, false}}, nil}
m := NewMethod("foo", "foo", Function, "", false, false, []Argument{{"bar", String, false}, {"baz", String, false}}, nil)
exp := "foo(string,string)"
if m.Sig() != exp {
t.Error("signature mismatch", exp, "!=", m.Sig())
if m.Sig != exp {
t.Error("signature mismatch", exp, "!=", m.Sig)
}

idexp := crypto.Keccak256([]byte(exp))[:4]
if !bytes.Equal(m.ID(), idexp) {
t.Errorf("expected ids to match %x != %x", m.ID(), idexp)
if !bytes.Equal(m.ID, idexp) {
t.Errorf("expected ids to match %x != %x", m.ID, idexp)
}

uintt, _ := NewType("uint256", "", nil)
m = Method{"foo", "foo", "", false, false, false, false, []Argument{{"bar", uintt, false}}, nil}
m = NewMethod("foo", "foo", Function, "", false, false, []Argument{{"bar", uintt, false}}, nil)
exp = "foo(uint256)"
if m.Sig() != exp {
t.Error("signature mismatch", exp, "!=", m.Sig())
if m.Sig != exp {
t.Error("signature mismatch", exp, "!=", m.Sig)
}

// Method with tuple arguments
Expand All @@ -204,10 +198,10 @@ func TestMethodSignature(t *testing.T) {
{Name: "y", Type: "int256"},
}},
})
m = Method{"foo", "foo", "", false, false, false, false, []Argument{{"s", s, false}, {"bar", String, false}}, nil}
m = NewMethod("foo", "foo", Function, "", false, false, []Argument{{"s", s, false}, {"bar", String, false}}, nil)
exp = "foo((int256,int256[],(int256,int256)[],(int256,int256)[2]),string)"
if m.Sig() != exp {
t.Error("signature mismatch", exp, "!=", m.Sig())
if m.Sig != exp {
t.Error("signature mismatch", exp, "!=", m.Sig)
}
}

Expand All @@ -219,12 +213,12 @@ func TestOverloadedMethodSignature(t *testing.T) {
}
check := func(name string, expect string, method bool) {
if method {
if abi.Methods[name].Sig() != expect {
t.Fatalf("The signature of overloaded method mismatch, want %s, have %s", expect, abi.Methods[name].Sig())
if abi.Methods[name].Sig != expect {
t.Fatalf("The signature of overloaded method mismatch, want %s, have %s", expect, abi.Methods[name].Sig)
}
} else {
if abi.Events[name].Sig() != expect {
t.Fatalf("The signature of overloaded event mismatch, want %s, have %s", expect, abi.Events[name].Sig())
if abi.Events[name].Sig != expect {
t.Fatalf("The signature of overloaded event mismatch, want %s, have %s", expect, abi.Events[name].Sig)
}
}
}
Expand Down Expand Up @@ -921,13 +915,13 @@ func TestABI_MethodById(t *testing.T) {
}
for name, m := range abi.Methods {
a := fmt.Sprintf("%v", m)
m2, err := abi.MethodById(m.ID())
m2, err := abi.MethodById(m.ID)
if err != nil {
t.Fatalf("Failed to look up ABI method: %v", err)
}
b := fmt.Sprintf("%v", m2)
if a != b {
t.Errorf("Method %v (id %x) not 'findable' by id in ABI", name, m.ID())
t.Errorf("Method %v (id %x) not 'findable' by id in ABI", name, m.ID)
}
}
// Also test empty
Expand Down Expand Up @@ -995,8 +989,8 @@ func TestABI_EventById(t *testing.T) {
t.Errorf("We should find a event for topic %s, test #%d", topicID.Hex(), testnum)
}

if event.ID() != topicID {
t.Errorf("Event id %s does not match topic %s, test #%d", event.ID().Hex(), topicID.Hex(), testnum)
if event.ID != topicID {
t.Errorf("Event id %s does not match topic %s, test #%d", event.ID.Hex(), topicID.Hex(), testnum)
}

unknowntopicID := crypto.Keccak256Hash([]byte("unknownEvent"))
Expand Down Expand Up @@ -1051,3 +1045,28 @@ func TestDoubleDuplicateMethodNames(t *testing.T) {
t.Fatalf("Should not have found extra method")
}
}

// TestUnnamedEventParam checks that an event with unnamed parameters is
// correctly handled
// The test runs the abi of the following contract.
// contract TestEvent {
// event send(uint256, uint256);
// }
func TestUnnamedEventParam(t *testing.T) {
abiJSON := `[{ "anonymous": false, "inputs": [{ "indexed": false,"internalType": "uint256", "name": "","type": "uint256"},{"indexed": false,"internalType": "uint256","name": "","type": "uint256"}],"name": "send","type": "event"}]`
contractAbi, err := JSON(strings.NewReader(abiJSON))
if err != nil {
t.Fatal(err)
}

event, ok := contractAbi.Events["send"]
if !ok {
t.Fatalf("Could not find event")
}
if event.Inputs[0].Name != "arg0" {
t.Fatalf("Could not find input")
}
if event.Inputs[1].Name != "arg1" {
t.Fatalf("Could not find input")
}
}
Loading

0 comments on commit fef9d4e

Please sign in to comment.