cmd/signer: fix bugs, improve abi detection, abi argument display

cmd/signer/abihelper.go

@@ -24,10 +24,12 @@ import (
 	"github.com/ethereum/go-ethereum/common"
 	"io/ioutil"
 	"strings"
+
+	"regexp"
 )
 
 type decodedArgument struct {
-	soltype string
+	soltype abi.Argument
 	value   interface{}
 }
 type decodedCallData struct {
@@ -36,9 +38,19 @@ type decodedCallData struct {
 	inputs    []decodedArgument
 }
 
+// String implements stringer interface, tries to use the underlying value-type
 func (arg decodedArgument) String() string {
-	return fmt.Sprintf("%v: %v", arg.soltype, arg.value)
+	var value string
+	switch arg.value.(type) {
+	case fmt.Stringer:
+		value = arg.value.(fmt.Stringer).String()
+	default:
+		value = fmt.Sprintf("%v", arg.value)
+	}
+	return fmt.Sprintf("%v: %v", arg.soltype.Type.String(), value)
 }
+
+// String implements stringer interface for decodedCallData
 func (cd decodedCallData) String() string {
 	args := make([]string, len(cd.inputs))
 	for i, arg := range cd.inputs {
@@ -55,16 +67,16 @@ func parseCallData(calldata []byte, abidata string) (*decodedCallData, error) {
 		return nil, fmt.Errorf("Invalid ABI-data, incomplete method signature of (%d bytes)", len(calldata))
 	}
 
-	abispec, err := abi.JSON(strings.NewReader(abidata))
-	if err != nil {
-		return nil, fmt.Errorf("Failed parsing JSON ABI: %v", err)
-	}
-
 	sigdata, argdata := calldata[:4], calldata[4:]
 	if len(argdata)%32 != 0 {
 		return nil, fmt.Errorf("Not ABI-encoded data; length should be a multiple of 32 (was %d)", len(argdata))
 	}
 
+	abispec, err := abi.JSON(strings.NewReader(abidata))
+	if err != nil {
+		return nil, fmt.Errorf("Failed parsing JSON ABI: %v, abidata: %v", err, abidata)
+	}
+
 	method := abispec.MethodById(sigdata)
 	if method == nil {
 		return nil, fmt.Errorf("Supplied ABI spec does not contain method signature in data: 0x%x", sigdata)
@@ -79,7 +91,7 @@ func parseCallData(calldata []byte, abidata string) (*decodedCallData, error) {
 			return nil, fmt.Errorf("Failed to decode argument %d (signature %v): %v", n, method.Sig(), err)
 		} else {
 			decodedArg := decodedArgument{
-				soltype: argument.Type.String(),
+				soltype: argument,
 				value:   value,
 			}
 			decoded.inputs = append(decoded.inputs, decodedArg)
@@ -102,15 +114,50 @@ func parseCallData(calldata []byte, abidata string) (*decodedCallData, error) {
 	if !bytes.Equal(encoded, calldata) {
 		exp := common.Bytes2Hex(encoded)
 		was := common.Bytes2Hex(calldata)
-		return nil, fmt.Errorf("WARNING: Supplied data is stuffed with extra data. %v \nWant %s\nHave %s", decoded, was, exp)
+		return nil, fmt.Errorf("WARNING: Supplied data is stuffed with extra data. \nWant %s\nHave %s\nfor method %v", exp, was, method.Sig())
 	}
 	return &decoded, nil
 }
 
+// MethodSelectorToAbi converts a method selector into an ABI struct. The returned data is a valid json string
+// which can be consumed by the standard abi package.
+func MethodSelectorToAbi(selector string) ([]byte, error) {
+
+	re := regexp.MustCompile("^([^\\)]+)\\(([a-z0-9,\\[\\]]*)\\)")
+
+	type fakeArg struct {
+		Type string `json:"type"`
+	}
+	type fakeABI struct {
+		Name   string    `json:"name"`
+		Type   string    `json:"type"`
+		Inputs []fakeArg `json:"inputs"`
+	}
+	groups := re.FindStringSubmatch(selector)
+	if len(groups) != 3 {
+		return nil, fmt.Errorf("Did not match: %v (%v matches)", selector, len(groups))
+	}
+	name := groups[1]
+	args := groups[2]
+	arguments := make([]fakeArg, 0)
+	if len(args) > 0 {
+		for _, arg := range strings.Split(args, ",") {
+			arguments = append(arguments, fakeArg{arg})
+		}
+	}
+	abicheat := fakeABI{
+		name, "function", arguments,
+	}
+	return json.Marshal([]fakeABI{abicheat})
+
+}
+
 type abiDb struct {
 	db map[string]string
 }
 
+// NewAbiDBFromFile loads signature database from file, and
+// errors if the file is not valid json. Does no other validation of contents
 func NewAbiDBFromFile(path string) (*abiDb, error) {
 	raw, err := ioutil.ReadFile(path)
 	if err != nil {
@@ -121,9 +168,9 @@ func NewAbiDBFromFile(path string) (*abiDb, error) {
 	return db, nil
 }
 
-// LookupABI checks the given 4byte-sequence against the known ABI methods.
+// LookupMethodSelector checks the given 4byte-sequence against the known ABI methods.
 // OBS: This method does not validate the match, it's assumed the caller will do so
-func (db *abiDb) LookupABI(id []byte) (string, error) {
+func (db *abiDb) LookupMethodSelector(id []byte) (string, error) {
 	if len(id) != 4 {
 		return "", fmt.Errorf("Expected 4-byte id, got %d", len(id))
 	}

cmd/signer/abihelper_test.go

@@ -17,7 +17,10 @@
 package main
 
 import (
+	"fmt"
+	"github.com/ethereum/go-ethereum/accounts/abi"
 	"github.com/ethereum/go-ethereum/common"
+	"strings"
 	"testing"
 )
 
@@ -84,3 +87,36 @@ func TestCalldataDecoding(t *testing.T) {
 		}
 	}
 }
+
+func TestSelectorUnmarshalling(t *testing.T) {
+	var (
+		db        *abiDb
+		err       error
+		abistring []byte
+		abistruct abi.ABI
+	)
+
+	db, err = NewAbiDBFromFile("4byte.json")
+	if err != nil {
+		t.Fatal(err)
+	}
+	fmt.Printf("DB size %v\n", db.Size())
+	for id, selector := range db.db {
+
+		abistring, err = MethodSelectorToAbi(selector)
+		if err != nil {
+			t.Error(err)
+			return
+		}
+		abistruct, err = abi.JSON(strings.NewReader(string(abistring)))
+		if err != nil {
+			t.Error(err)
+			return
+		}
+		m := abistruct.MethodById(common.Hex2Bytes(id[2:]))
+		if m.Sig() != selector {
+			t.Errorf("Expected equality: %v != %v", m.Sig(), selector)
+		}
+	}
+
+}

cmd/signer/api.go

@@ -56,7 +56,7 @@ type (
 	SignTxRequest struct {
 		Transaction TransactionArg `json:"transaction"`
 		From        common.Address `json:"fromaccount"`
-		Callinfo    fmt.Stringer   `json:"call_info"`
+		Callinfo    string         `json:"call_info"`
 		Meta        Metadata       `json:"meta"`
 	}
 	// SignTxResponse result from SignTxRequest
@@ -299,7 +299,7 @@ func logDiff(original *SignTxRequest, new *SignTxResponse) bool {
 
 // SignTransaction signs the given Transaction and returns it in an RLP encoded form
 // that can be posted to `eth_sendRawTransaction`.
-func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address, args TransactionArg, methodSig *string) (hexutil.Bytes, error) {
+func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address, args TransactionArg, methodSelector *string) (hexutil.Bytes, error) {
 
 	var (
 		err    error
@@ -311,19 +311,27 @@ func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address,
 	data := args.Data
 	if len(data) > 3 {
 		// Try to make sense of the data
-		var abidata string
-		if methodSig == nil {
-			abidata, err = api.abidb.LookupABI(data[:4])
+		var selector string
+		if methodSelector == nil {
+			selector, err = api.abidb.LookupMethodSelector(data[:4])
 			if err != nil {
-				req.Callinfo = errorWrapper{"Warning! Could not locate ABI", err}
+				req.Callinfo = errorWrapper{"Warning! Could not locate ABI", err}.String()
 			}
 		} else {
-			abidata = *methodSig
+			selector = *methodSelector
 		}
-		if abidata != "" {
-			req.Callinfo, err = parseCallData(data, abidata)
+		if selector != "" {
+			abidata, err := MethodSelectorToAbi(selector)
 			if err != nil {
-				req.Callinfo = errorWrapper{"Warning! Could not validate ABI-data against calldata", err}
+				req.Callinfo = errorWrapper{"Warning! Could not validate ABI-data against calldata", err}.String()
+			} else {
+				var info *decodedCallData
+				info, err = parseCallData(data, string(abidata))
+				if err != nil {
+					req.Callinfo = errorWrapper{"Warning! Could not validate ABI-data against calldata", err}.String()
+				} else {
+					req.Callinfo = info.String()
+				}
 			}
 		}
 	}
@@ -338,8 +346,12 @@ func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address,
 	// Log changes made by the UI to the signing-request
 	logDiff(&req, &result)
 
-	acc := accounts.Account{Address: result.From}
-	wallet, err := api.am.Find(acc)
+	var (
+		acc    accounts.Account
+		wallet accounts.Wallet
+	)
+	acc = accounts.Account{Address: result.From}
+	wallet, err = api.am.Find(acc)
 	if err != nil {
 		return nil, err
 	}

cmd/signer/api_test.go

@@ -8,6 +8,8 @@ import (
 	"github.com/ethereum/go-ethereum/cmd/utils"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
+	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/rlp"
 	"io/ioutil"
 	"math/big"
 	"os"
@@ -291,10 +293,12 @@ func TestSignTx(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if h == nil || len(h) != 118 {
-		t.Errorf("Expected 181 byte rlp-data (got %d bytes)", len(h))
+	parsedTx := &types.Transaction{}
+	rlp.Decode(bytes.NewReader(h), parsedTx)
+	//The tx should NOT be modified by the UI
+	if parsedTx.Value().Cmp(tx.Value.ToInt()) != 0 {
+		t.Errorf("Expected value to be unchanged, expected %v got %v", tx.Value, parsedTx.Value())
 	}
-	//The tx is NOT modified by the UI
 	control <- "Y"
 	control <- "apassword"
 
@@ -314,6 +318,14 @@ func TestSignTx(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+
+	parsedTx2 := &types.Transaction{}
+	rlp.Decode(bytes.NewReader(h), parsedTx2)
+	//The tx should NOT be modified by the UI
+	if parsedTx2.Value().Cmp(tx.Value.ToInt()) != 0 {
+		t.Errorf("Expected value to be changed, got %v", parsedTx.Value())
+	}
+
 	if bytes.Equal(h, h2) {
 		t.Error("Expected tx to be modified by UI")
 	}

cmd/signer/cliui.go

@@ -97,24 +97,29 @@ func showMetadata(metadata Metadata) {
 func (ui *CommandlineUI) ApproveTx(request *SignTxRequest) (SignTxResponse, error) {
 	ui.mu.Lock()
 	defer ui.mu.Unlock()
-	weival := request.Transaction.Value
-
+	weival := request.Transaction.Value.ToInt()
+	toval := ""
+	if request.Transaction.To != nil {
+		toval = request.Transaction.To.Hex()
+	}
 	fmt.Printf("--------- Transaction request-------------\n")
-	fmt.Printf("to:    %v\n", request.Transaction.To)
+	fmt.Printf("to:    %v\n", toval)
 	fmt.Printf("from:  %v\n", request.From.Hex())
 	fmt.Printf("value: %v wei\n", weival)
 	if len(request.Transaction.Data) > 0 {
 		fmt.Printf("data:  %v\n", common.Bytes2Hex(request.Transaction.Data))
 	}
-	if request.Callinfo != nil {
+	if request.Callinfo != "" {
 		fmt.Printf("\nNote: This Transaction contains data. Review abi-decoding info below:")
-		fmt.Printf("\nCall info:\n\t%v\n", request.Callinfo.String())
+		fmt.Printf("\nCall info:\n\t%v\n", request.Callinfo)
 
 	}
 	fmt.Printf("\n")
 	showMetadata(request.Meta)
 	fmt.Printf("-------------------------------------------\n")
-
+	if !ui.confirm() {
+		return SignTxResponse{request.Transaction, request.From, false, ""}, nil
+	}
 	return SignTxResponse{request.Transaction, request.From, true, ui.readPassword()}, nil
 }
 
@@ -130,6 +135,9 @@ func (ui *CommandlineUI) ApproveSignData(request *SignDataRequest) (SignDataResp
 	fmt.Printf("message hash:  %v\n", request.Hash)
 	fmt.Printf("-------------------------------------------\n")
 	showMetadata(request.Meta)
+	if !ui.confirm() {
+		return SignDataResponse{false, ""}, nil
+	}
 	return SignDataResponse{true, ui.readPassword()}, nil
 }
 
@@ -196,7 +204,10 @@ func (ui *CommandlineUI) ApproveNewAccount(request *NewAccountRequest) (NewAccou
 	fmt.Printf("Approving this operation means that a new Account is created,\n")
 	fmt.Printf("and the address show to the caller\n")
 	showMetadata(request.Meta)
-	return NewAccountResponse{ui.confirm(), ui.readPassword()}, nil
+	if !ui.confirm() {
+		return NewAccountResponse{false, ""}, nil
+	}
+	return NewAccountResponse{true, ui.readPassword()}, nil
 }
 
 // ShowError displays error message to user

cmd/signer/main.go

@@ -183,18 +183,18 @@ func testExternalUI(api *SignerAPI) {
 	api.ui.ShowInfo("Testing 'ShowInfo'")
 	api.ui.ShowError("Testing 'ShowError'")
 
-	checkErr:= func(method string, err error){
-		if err != nil && err != ErrRequestDenied{
+	checkErr := func(method string, err error) {
+		if err != nil && err != ErrRequestDenied {
 			errs = append(errs, fmt.Sprintf("%v: %v", method, err.Error()))
 		}
 	}
 	var err error
 
-	_, err = api.SignTransaction(ctx, common.Address{}, TransactionArg{}, nil);
+	_, err = api.SignTransaction(ctx, common.Address{}, TransactionArg{}, nil)
 	checkErr("SignTransaction", err)
 	_, err = api.Sign(ctx, common.Address{}, common.Hex2Bytes("01020304"))
 	checkErr("Sign", err)
-	_, err =api.List(ctx)
+	_, err = api.List(ctx)
 	checkErr("List", err)
 	_, err = api.New(ctx)
 	checkErr("New", err)
@@ -210,7 +210,7 @@ func testExternalUI(api *SignerAPI) {
 		for _, e := range errs {
 			log.Error(e)
 		}
-	}else{
+	} else {
 		log.Info("No errors")
 	}
 

cmd/signer/stdioui.go

@@ -111,7 +111,7 @@ func (ui StdIOUI) ApproveNewAccount(request *NewAccountRequest) (NewAccountRespo
 func (ui StdIOUI) ShowError(message string) {
 	err := ui.dispatch("ShowError", &Message{message}, nil)
 	if err != nil {
-		log.Info("Error calling 'ShowError'", "exc", err.Error(),"msg", message)
+		log.Info("Error calling 'ShowError'", "exc", err.Error(), "msg", message)
 	}
 }