cmd/clef, signer: support for eip-1559 txs in clef (#22966)

holiman · web-flow · commit 7a00378e2baa · 2021-06-09T13:48:47.000+02:00
diff --git a/accounts/external/backend.go b/accounts/external/backend.go
@@ -204,13 +204,18 @@ func (api *ExternalSigner) SignTx(account accounts.Account, tx *types.Transactio
 		to = &t
 	}
 	args := &core.SendTxArgs{
-		Data:     &data,
-		Nonce:    hexutil.Uint64(tx.Nonce()),
-		Value:    hexutil.Big(*tx.Value()),
-		Gas:      hexutil.Uint64(tx.Gas()),
-		GasPrice: hexutil.Big(*tx.GasPrice()),
-		To:       to,
-		From:     common.NewMixedcaseAddress(account.Address),
+		Data:  &data,
+		Nonce: hexutil.Uint64(tx.Nonce()),
+		Value: hexutil.Big(*tx.Value()),
+		Gas:   hexutil.Uint64(tx.Gas()),
+		To:    to,
+		From:  common.NewMixedcaseAddress(account.Address),
+	}
+	if tx.GasFeeCap() != nil {
+		args.MaxFeePerGas = (*hexutil.Big)(tx.GasFeeCap())
+		args.MaxPriorityFeePerGas = (*hexutil.Big)(tx.GasTipCap())
+	} else {
+		args.GasPrice = (*hexutil.Big)(tx.GasPrice())
 	}
 	// We should request the default chain id that we're operating with
 	// (the chain we're executing on)
diff --git a/cmd/clef/main.go b/cmd/clef/main.go
@@ -929,7 +929,7 @@ func testExternalUI(api *core.SignerAPI) {
 			Value:    hexutil.Big(*big.NewInt(6)),
 			From:     common.NewMixedcaseAddress(a),
 			To:       &to,
-			GasPrice: hexutil.Big(*big.NewInt(5)),
+			GasPrice: (*hexutil.Big)(big.NewInt(5)),
 			Gas:      1000,
 			Input:    nil,
 		}
@@ -1065,7 +1065,7 @@ func GenDoc(ctx *cli.Context) {
 				Value:    hexutil.Big(*big.NewInt(6)),
 				From:     common.NewMixedcaseAddress(a),
 				To:       nil,
-				GasPrice: hexutil.Big(*big.NewInt(5)),
+				GasPrice: (*hexutil.Big)(big.NewInt(5)),
 				Gas:      1000,
 				Input:    nil,
 			}})
@@ -1081,7 +1081,7 @@ func GenDoc(ctx *cli.Context) {
 					Value:    hexutil.Big(*big.NewInt(6)),
 					From:     common.NewMixedcaseAddress(a),
 					To:       nil,
-					GasPrice: hexutil.Big(*big.NewInt(5)),
+					GasPrice: (*hexutil.Big)(big.NewInt(5)),
 					Gas:      1000,
 					Input:    nil,
 				}})
diff --git a/cmd/clef/testdata/sign_1559_missing_field_exp_fail.json b/cmd/clef/testdata/sign_1559_missing_field_exp_fail.json
@@ -0,0 +1,16 @@
+{
+  "jsonrpc": "2.0",
+  "method": "account_signTransaction",
+  "params": [
+    {
+      "from": "0x8A8eAFb1cf62BfBeb1741769DAE1a9dd47996192",
+      "to": "0x8A8eAFb1cf62BfBeb1741769DAE1a9dd47996192",
+      "gas": "0x333",
+      "maxFeePerGas": "0x123",
+      "nonce": "0x0",
+      "value": "0x10",
+      "data": "0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"
+    }
+  ],
+  "id": 67
+}
diff --git a/cmd/clef/testdata/sign_1559_missing_maxfeepergas_exp_fail.json b/cmd/clef/testdata/sign_1559_missing_maxfeepergas_exp_fail.json
@@ -0,0 +1,16 @@
+{
+  "jsonrpc": "2.0",
+  "method": "account_signTransaction",
+  "params": [
+    {
+      "from": "0x8A8eAFb1cf62BfBeb1741769DAE1a9dd47996192",
+      "to": "0x8A8eAFb1cf62BfBeb1741769DAE1a9dd47996192",
+      "gas": "0x333",
+      "maxPriorityFeePerGas": "0x123",
+      "nonce": "0x0",
+      "value": "0x10",
+      "data": "0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"
+    }
+  ],
+  "id": 67
+}
diff --git a/cmd/clef/testdata/sign_1559_tx.json b/cmd/clef/testdata/sign_1559_tx.json
@@ -0,0 +1,17 @@
+{
+  "jsonrpc": "2.0",
+  "method": "account_signTransaction",
+  "params": [
+    {
+      "from": "0x8A8eAFb1cf62BfBeb1741769DAE1a9dd47996192",
+      "to": "0x8A8eAFb1cf62BfBeb1741769DAE1a9dd47996192",
+      "gas": "0x333",
+      "maxPriorityFeePerGas": "0x123",
+      "maxFeePerGas": "0x123",
+      "nonce": "0x0",
+      "value": "0x10",
+      "data": "0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"
+    }
+  ],
+  "id": 67
+}
diff --git a/cmd/clef/testdata/sign_bad_checksum_exp_fail.json b/cmd/clef/testdata/sign_bad_checksum_exp_fail.json
@@ -0,0 +1,17 @@
+{
+  "jsonrpc": "2.0",
+  "method": "account_signTransaction",
+  "params": [
+    {
+	"from":"0x8a8eafb1cf62bfbeb1741769dae1a9dd47996192",
+	"to":"0x8a8eafb1cf62bfbeb1741769dae1a9dd47996192",
+      "gas": "0x333",
+      "gasPrice": "0x123",
+      "nonce": "0x0",
+      "value": "0x10",
+      "data":
+        "0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"
+    }
+  ],
+  "id": 67
+}
diff --git a/cmd/clef/testdata/sign_normal_exp_ok.json b/cmd/clef/testdata/sign_normal_exp_ok.json
@@ -0,0 +1,17 @@
+{
+  "jsonrpc": "2.0",
+  "method": "account_signTransaction",
+  "params": [
+    {
+	"from":"0x8A8eAFb1cf62BfBeb1741769DAE1a9dd47996192",
+	"to":"0x8A8eAFb1cf62BfBeb1741769DAE1a9dd47996192",
+      "gas": "0x333",
+      "gasPrice": "0x123",
+      "nonce": "0x0",
+      "value": "0x10",
+      "data":
+        "0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"
+    }
+  ],
+  "id": 67
+}
diff --git a/internal/ethapi/transaction_args.go b/internal/ethapi/transaction_args.go
@@ -265,3 +265,9 @@ func (args *TransactionArgs) toTransaction() *types.Transaction {
 	}
 	return types.NewTx(data)
 }
+
+// ToTransaction converts the arguments to a transaction.
+// This assumes that setDefaults has been called.
+func (args *TransactionArgs) ToTransaction() *types.Transaction {
+	return args.toTransaction()
+}
diff --git a/signer/core/api.go b/signer/core/api.go
@@ -456,6 +456,16 @@ func (api *SignerAPI) newAccount() (common.Address, error) {
 // it also returns 'true' if the transaction was modified, to make it possible to configure the signer not to allow
 // UI-modifications to requests
 func logDiff(original *SignTxRequest, new *SignTxResponse) bool {
+	var intPtrModified = func(a, b *hexutil.Big) bool {
+		aBig := (*big.Int)(a)
+		bBig := (*big.Int)(b)
+		if aBig != nil && bBig != nil {
+			return aBig.Cmp(bBig) != 0
+		}
+		// One or both of them are nil
+		return a != b
+	}
+
 	modified := false
 	if f0, f1 := original.Transaction.From, new.Transaction.From; !reflect.DeepEqual(f0, f1) {
 		log.Info("Sender-account changed by UI", "was", f0, "is", f1)
@@ -469,9 +479,17 @@ func logDiff(original *SignTxRequest, new *SignTxResponse) bool {
 		modified = true
 		log.Info("Gas changed by UI", "was", g0, "is", g1)
 	}
-	if g0, g1 := big.Int(original.Transaction.GasPrice), big.Int(new.Transaction.GasPrice); g0.Cmp(&g1) != 0 {
+	if a, b := original.Transaction.GasPrice, new.Transaction.GasPrice; intPtrModified(a, b) {
+		log.Info("GasPrice changed by UI", "was", a, "is", b)
+		modified = true
+	}
+	if a, b := original.Transaction.MaxPriorityFeePerGas, new.Transaction.MaxPriorityFeePerGas; intPtrModified(a, b) {
+		log.Info("maxPriorityFeePerGas changed by UI", "was", a, "is", b)
+		modified = true
+	}
+	if a, b := original.Transaction.MaxFeePerGas, new.Transaction.MaxFeePerGas; intPtrModified(a, b) {
+		log.Info("maxFeePerGas changed by UI", "was", a, "is", b)
 		modified = true
-		log.Info("GasPrice changed by UI", "was", g0, "is", g1)
 	}
 	if v0, v1 := big.Int(original.Transaction.Value), big.Int(new.Transaction.Value); v0.Cmp(&v1) != 0 {
 		modified = true
diff --git a/signer/core/api_test.go b/signer/core/api_test.go
@@ -234,7 +234,7 @@ func mkTestTx(from common.MixedcaseAddress) core.SendTxArgs {
 		From:     from,
 		To:       &to,
 		Gas:      gas,
-		GasPrice: gasPrice,
+		GasPrice: &gasPrice,
 		Value:    value,
 		Data:     &data,
 		Nonce:    nonce}
diff --git a/signer/core/cliui.go b/signer/core/cliui.go
@@ -113,10 +113,15 @@ func (ui *CommandlineUI) ApproveTx(request *SignTxRequest) (SignTxResponse, erro
 	} else {
 		fmt.Printf("to:    <contact creation>\n")
 	}
-	fmt.Printf("from:     %v\n", request.Transaction.From.String())
-	fmt.Printf("value:    %v wei\n", weival)
-	fmt.Printf("gas:      %v (%v)\n", request.Transaction.Gas, uint64(request.Transaction.Gas))
-	fmt.Printf("gasprice: %v wei\n", request.Transaction.GasPrice.ToInt())
+	fmt.Printf("from:               %v\n", request.Transaction.From.String())
+	fmt.Printf("value:              %v wei\n", weival)
+	fmt.Printf("gas:                %v (%v)\n", request.Transaction.Gas, uint64(request.Transaction.Gas))
+	if request.Transaction.MaxFeePerGas != nil {
+		fmt.Printf("maxFeePerGas:          %v wei\n", request.Transaction.MaxFeePerGas.ToInt())
+		fmt.Printf("maxPriorityFeePerGas:  %v wei\n", request.Transaction.MaxPriorityFeePerGas.ToInt())
+	} else {
+		fmt.Printf("gasprice: %v wei\n", request.Transaction.GasPrice.ToInt())
+	}
 	fmt.Printf("nonce:    %v (%v)\n", request.Transaction.Nonce, uint64(request.Transaction.Nonce))
 	if chainId := request.Transaction.ChainID; chainId != nil {
 		fmt.Printf("chainid:  %v\n", chainId)
diff --git a/signer/core/gnosis_safe.go b/signer/core/gnosis_safe.go
@@ -77,11 +77,12 @@ func (tx *GnosisSafeTx) ToTypedData() TypedData {
 // ArgsForValidation returns a SendTxArgs struct, which can be used for the
 // common validations, e.g. look up 4byte destinations
 func (tx *GnosisSafeTx) ArgsForValidation() *SendTxArgs {
+	gp := hexutil.Big(tx.GasPrice)
 	args := &SendTxArgs{
 		From:     tx.Safe,
 		To:       &tx.To,
 		Gas:      hexutil.Uint64(tx.SafeTxGas.Uint64()),
-		GasPrice: hexutil.Big(tx.GasPrice),
+		GasPrice: &gp,
 		Value:    hexutil.Big(tx.Value),
 		Nonce:    hexutil.Uint64(tx.Nonce.Uint64()),
 		Data:     tx.Data,
diff --git a/signer/core/types.go b/signer/core/types.go
@@ -19,12 +19,12 @@ package core
 import (
 	"encoding/json"
 	"fmt"
-	"math/big"
 	"strings"
 
 	"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/internal/ethapi"
 )
 
 type ValidationInfo struct {
@@ -66,14 +66,21 @@ func (v *ValidationMessages) getWarnings() error {
 }
 
 // SendTxArgs represents the arguments to submit a transaction
+// This struct is identical to ethapi.TransactionArgs, except for the usage of
+// common.MixedcaseAddress in From and To
 type SendTxArgs struct {
-	From     common.MixedcaseAddress  `json:"from"`
-	To       *common.MixedcaseAddress `json:"to"`
-	Gas      hexutil.Uint64           `json:"gas"`
-	GasPrice hexutil.Big              `json:"gasPrice"`
-	Value    hexutil.Big              `json:"value"`
-	Nonce    hexutil.Uint64           `json:"nonce"`
+	From                 common.MixedcaseAddress  `json:"from"`
+	To                   *common.MixedcaseAddress `json:"to"`
+	Gas                  hexutil.Uint64           `json:"gas"`
+	GasPrice             *hexutil.Big             `json:"gasPrice"`
+	MaxFeePerGas         *hexutil.Big             `json:"maxFeePerGas"`
+	MaxPriorityFeePerGas *hexutil.Big             `json:"maxPriorityFeePerGas"`
+	Value                hexutil.Big              `json:"value"`
+	Nonce                hexutil.Uint64           `json:"nonce"`
+
 	// We accept "data" and "input" for backwards-compatibility reasons.
+	// "input" is the newer name and should be preferred by clients.
+	// Issue detail: https://github.com/ethereum/go-ethereum/issues/15628
 	Data  *hexutil.Bytes `json:"data"`
 	Input *hexutil.Bytes `json:"input,omitempty"`
 
@@ -91,38 +98,22 @@ func (args SendTxArgs) String() string {
 }
 
 func (args *SendTxArgs) toTransaction() *types.Transaction {
-	var input []byte
-	if args.Data != nil {
-		input = *args.Data
-	} else if args.Input != nil {
-		input = *args.Input
+	txArgs := ethapi.TransactionArgs{
+		Gas:                  &args.Gas,
+		GasPrice:             args.GasPrice,
+		MaxFeePerGas:         args.MaxFeePerGas,
+		MaxPriorityFeePerGas: args.MaxPriorityFeePerGas,
+		Value:                &args.Value,
+		Nonce:                &args.Nonce,
+		Data:                 args.Data,
+		Input:                args.Input,
+		AccessList:           args.AccessList,
+		ChainID:              args.ChainID,
 	}
-	var to *common.Address
+	// Add the To-field, if specified
 	if args.To != nil {
-		_to := args.To.Address()
-		to = &_to
-	}
-	var data types.TxData
-	if args.AccessList == nil {
-		data = &types.LegacyTx{
-			To:       to,
-			Nonce:    uint64(args.Nonce),
-			Gas:      uint64(args.Gas),
-			GasPrice: (*big.Int)(&args.GasPrice),
-			Value:    (*big.Int)(&args.Value),
-			Data:     input,
-		}
-	} else {
-		data = &types.AccessListTx{
-			To:         to,
-			ChainID:    (*big.Int)(args.ChainID),
-			Nonce:      uint64(args.Nonce),
-			Gas:        uint64(args.Gas),
-			GasPrice:   (*big.Int)(&args.GasPrice),
-			Value:      (*big.Int)(&args.Value),
-			Data:       input,
-			AccessList: *args.AccessList,
-		}
+		to := args.To.Address()
+		txArgs.To = &to
 	}
-	return types.NewTx(data)
+	return txArgs.ToTransaction()
 }
diff --git a/signer/fourbyte/validation.go b/signer/fourbyte/validation.go
@@ -73,6 +73,16 @@ func (db *Database) ValidateTransaction(selector *string, tx *core.SendTxArgs) (
 	if bytes.Equal(tx.To.Address().Bytes(), common.Address{}.Bytes()) {
 		messages.Crit("Transaction recipient is the zero address")
 	}
+	switch {
+	case tx.GasPrice == nil && tx.MaxFeePerGas == nil:
+		messages.Crit("Neither 'gasPrice' nor 'maxFeePerGas' specified.")
+	case tx.GasPrice == nil && tx.MaxPriorityFeePerGas == nil:
+		messages.Crit("Neither 'gasPrice' nor 'maxPriorityFeePerGas' specified.")
+	case tx.GasPrice != nil && tx.MaxFeePerGas != nil:
+		messages.Crit("Both 'gasPrice' and 'maxFeePerGas' specified.")
+	case tx.GasPrice != nil && tx.MaxPriorityFeePerGas != nil:
+		messages.Crit("Both 'gasPrice' and 'maxPriorityFeePerGas' specified.")
+	}
 	// Semantic fields validated, try to make heads or tails of the call data
 	db.ValidateCallData(selector, data, messages)
 	return messages, nil
diff --git a/signer/fourbyte/validation_test.go b/signer/fourbyte/validation_test.go
@@ -60,7 +60,7 @@ func dummyTxArgs(t txtestcase) *core.SendTxArgs {
 		To:       to,
 		Value:    value,
 		Nonce:    n,
-		GasPrice: gasPrice,
+		GasPrice: &gasPrice,
 		Gas:      gas,
 		Data:     data,
 		Input:    input,
diff --git a/signer/rules/rules_test.go b/signer/rules/rules_test.go
@@ -437,7 +437,7 @@ func dummyTx(value hexutil.Big) *core.SignTxRequest {
 			To:       to,
 			Value:    value,
 			Nonce:    n,
-			GasPrice: gasPrice,
+			GasPrice: &gasPrice,
 			Gas:      gas,
 		},
 		Callinfo: []core.ValidationInfo{

Original file line number	Diff line number	Diff line change
`@@ -265,3 +265,9 @@ func (args TransactionArgs) toTransaction() types.Transaction {`
`265`	`265`	`}`
`266`	`266`	`return types.NewTx(data)`
`267`	`267`	`}`
	`268`	`+`
	`269`	`+// ToTransaction converts the arguments to a transaction.`
	`270`	`+// This assumes that setDefaults has been called.`
	`271`	`+func (args TransactionArgs) ToTransaction() types.Transaction {`
	`272`	`+ return args.toTransaction()`
	`273`	`+}`