From 7a00378e2baa2430d997f2dab9e29a9082cbf32a Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 9 Jun 2021 13:48:47 +0200 Subject: [PATCH] cmd/clef, signer: support for eip-1559 txs in clef (#22966) --- accounts/external/backend.go | 19 ++++-- cmd/clef/main.go | 6 +- .../sign_1559_missing_field_exp_fail.json | 16 +++++ ...gn_1559_missing_maxfeepergas_exp_fail.json | 16 +++++ cmd/clef/testdata/sign_1559_tx.json | 17 +++++ .../testdata/sign_bad_checksum_exp_fail.json | 17 +++++ cmd/clef/testdata/sign_normal_exp_ok.json | 17 +++++ internal/ethapi/transaction_args.go | 6 ++ signer/core/api.go | 22 +++++- signer/core/api_test.go | 2 +- signer/core/cliui.go | 13 ++-- signer/core/gnosis_safe.go | 3 +- signer/core/types.go | 67 ++++++++----------- signer/fourbyte/validation.go | 10 +++ signer/fourbyte/validation_test.go | 2 +- signer/rules/rules_test.go | 2 +- 16 files changed, 177 insertions(+), 58 deletions(-) create mode 100644 cmd/clef/testdata/sign_1559_missing_field_exp_fail.json create mode 100644 cmd/clef/testdata/sign_1559_missing_maxfeepergas_exp_fail.json create mode 100644 cmd/clef/testdata/sign_1559_tx.json create mode 100644 cmd/clef/testdata/sign_bad_checksum_exp_fail.json create mode 100644 cmd/clef/testdata/sign_normal_exp_ok.json diff --git a/accounts/external/backend.go b/accounts/external/backend.go index 59766217d2..4f6ca39967 100644 --- 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 index 8befce88dc..84e1dda99e 100644 --- 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 new file mode 100644 index 0000000000..c5a1336860 --- /dev/null +++ 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 new file mode 100644 index 0000000000..df69231d7e --- /dev/null +++ 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 new file mode 100644 index 0000000000..29355f6cf5 --- /dev/null +++ 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 new file mode 100644 index 0000000000..21ba7b3fc0 --- /dev/null +++ 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 new file mode 100644 index 0000000000..7f3a9202a0 --- /dev/null +++ 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 index b96229efd2..b94bb9ac44 100644 --- 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 index 3811162f8f..03daeeb6e7 100644 --- 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 index 800020b0cf..ff4d281d45 100644 --- 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 index e0375483c3..05c60906cc 100644 --- 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: \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 index e4385f9dc3..050425f0af 100644 --- 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 index e952a21209..85ea6170d6 100644 --- 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 + to := args.To.Address() + txArgs.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, - } - } - return types.NewTx(data) + return txArgs.ToTransaction() } diff --git a/signer/fourbyte/validation.go b/signer/fourbyte/validation.go index baec32f72c..f311c89e52 100644 --- 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 index 0e98cd88e4..b088cf3097 100644 --- 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 index 510c57e67f..ec11e37121 100644 --- 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{