From 3cdec8a80c1984c3bca4305b3cf49d3f160679ad Mon Sep 17 00:00:00 2001 From: Yondon Fu Date: Wed, 29 Sep 2021 21:24:48 -0400 Subject: [PATCH 1/5] accounts/abi/bind: Do not overwrite TransactOpts in transact --- accounts/abi/bind/base.go | 39 ++++++++------- accounts/abi/bind/base_test.go | 86 ++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 18 deletions(-) diff --git a/accounts/abi/bind/base.go b/accounts/abi/bind/base.go index 6d99517e67cf7..5add5de0d57e5 100644 --- a/accounts/abi/bind/base.go +++ b/accounts/abi/bind/base.go @@ -250,42 +250,45 @@ func (c *BoundContract) transact(opts *TransactOpts, contract *common.Address, i } else { nonce = opts.Nonce.Uint64() } + // Figure out reasonable gas price values - if opts.GasPrice != nil && (opts.GasFeeCap != nil || opts.GasTipCap != nil) { + gasPrice := opts.GasPrice + gasTipCap := opts.GasTipCap + gasFeeCap := opts.GasFeeCap + if gasPrice != nil && (gasFeeCap != nil || gasTipCap != nil) { return nil, errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified") } head, err := c.transactor.HeaderByNumber(ensureContext(opts.Context), nil) if err != nil { return nil, err } - if head.BaseFee != nil && opts.GasPrice == nil { - if opts.GasTipCap == nil { + if head.BaseFee != nil && gasPrice == nil { + if gasTipCap == nil { tip, err := c.transactor.SuggestGasTipCap(ensureContext(opts.Context)) if err != nil { return nil, err } - opts.GasTipCap = tip + gasTipCap = tip } - if opts.GasFeeCap == nil { - gasFeeCap := new(big.Int).Add( - opts.GasTipCap, + if gasFeeCap == nil { + gasFeeCap = new(big.Int).Add( + gasTipCap, new(big.Int).Mul(head.BaseFee, big.NewInt(2)), ) - opts.GasFeeCap = gasFeeCap } - if opts.GasFeeCap.Cmp(opts.GasTipCap) < 0 { - return nil, fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", opts.GasFeeCap, opts.GasTipCap) + if gasFeeCap.Cmp(gasTipCap) < 0 { + return nil, fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", gasFeeCap, gasTipCap) } } else { - if opts.GasFeeCap != nil || opts.GasTipCap != nil { + if gasFeeCap != nil || gasTipCap != nil { return nil, errors.New("maxFeePerGas or maxPriorityFeePerGas specified but london is not active yet") } - if opts.GasPrice == nil { + if gasPrice == nil { price, err := c.transactor.SuggestGasPrice(ensureContext(opts.Context)) if err != nil { return nil, err } - opts.GasPrice = price + gasPrice = price } } gasLimit := opts.GasLimit @@ -299,7 +302,7 @@ func (c *BoundContract) transact(opts *TransactOpts, contract *common.Address, i } } // If the contract surely has code (or code is not needed), estimate the transaction - msg := ethereum.CallMsg{From: opts.From, To: contract, GasPrice: opts.GasPrice, GasTipCap: opts.GasTipCap, GasFeeCap: opts.GasFeeCap, Value: value, Data: input} + msg := ethereum.CallMsg{From: opts.From, To: contract, GasPrice: gasPrice, GasTipCap: gasTipCap, GasFeeCap: gasFeeCap, Value: value, Data: input} gasLimit, err = c.transactor.EstimateGas(ensureContext(opts.Context), msg) if err != nil { return nil, fmt.Errorf("failed to estimate gas needed: %v", err) @@ -307,10 +310,10 @@ func (c *BoundContract) transact(opts *TransactOpts, contract *common.Address, i } // Create the transaction, sign it and schedule it for execution var rawTx *types.Transaction - if opts.GasFeeCap == nil { + if gasFeeCap == nil { baseTx := &types.LegacyTx{ Nonce: nonce, - GasPrice: opts.GasPrice, + GasPrice: gasPrice, Gas: gasLimit, Value: value, Data: input, @@ -322,8 +325,8 @@ func (c *BoundContract) transact(opts *TransactOpts, contract *common.Address, i } else { baseTx := &types.DynamicFeeTx{ Nonce: nonce, - GasFeeCap: opts.GasFeeCap, - GasTipCap: opts.GasTipCap, + GasFeeCap: gasFeeCap, + GasTipCap: gasTipCap, Gas: gasLimit, Value: value, Data: input, diff --git a/accounts/abi/bind/base_test.go b/accounts/abi/bind/base_test.go index 7b023e3b48405..08ba18f95e545 100644 --- a/accounts/abi/bind/base_test.go +++ b/accounts/abi/bind/base_test.go @@ -31,8 +31,49 @@ import ( "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/rlp" + "github.com/stretchr/testify/assert" ) +func mockSign(addr common.Address, tx *types.Transaction) (*types.Transaction, error) { return tx, nil } + +type mockTransactor struct { + baseFee *big.Int + gasTipCap *big.Int + gasPrice *big.Int + suggestGasTipCapCalled bool + suggestGasPriceCalled bool +} + +func (mt *mockTransactor) HeaderByNumber(ctx context.Context, number *big.Int) (*types.Header, error) { + return &types.Header{BaseFee: mt.baseFee}, nil +} + +func (mt *mockTransactor) PendingCodeAt(ctx context.Context, account common.Address) ([]byte, error) { + return []byte{1}, nil +} + +func (mt *mockTransactor) PendingNonceAt(ctx context.Context, account common.Address) (uint64, error) { + return 0, nil +} + +func (mt *mockTransactor) SuggestGasPrice(ctx context.Context) (*big.Int, error) { + mt.suggestGasPriceCalled = true + return mt.gasPrice, nil +} + +func (mt *mockTransactor) SuggestGasTipCap(ctx context.Context) (*big.Int, error) { + mt.suggestGasTipCapCalled = true + return mt.gasTipCap, nil +} + +func (mt *mockTransactor) EstimateGas(ctx context.Context, call ethereum.CallMsg) (gas uint64, err error) { + return 0, nil +} + +func (mt *mockTransactor) SendTransaction(ctx context.Context, tx *types.Transaction) error { + return nil +} + type mockCaller struct { codeAtBlockNumber *big.Int callContractBlockNumber *big.Int @@ -226,6 +267,51 @@ func TestUnpackIndexedBytesTyLogIntoMap(t *testing.T) { unpackAndCheck(t, bc, expectedReceivedMap, mockLog) } +func TestTransactGasFee(t *testing.T) { + assert := assert.New(t) + + // GasTipCap and GasFeeCap + // When opts.GasTipCap and opts.GasFeeCap are nil + mt := &mockTransactor{baseFee: big.NewInt(100), gasTipCap: big.NewInt(5)} + bc := bind.NewBoundContract(common.Address{}, abi.ABI{}, nil, mt, nil) + opts := &bind.TransactOpts{Signer: mockSign} + tx, err := bc.Transact(opts, "") + assert.Nil(err) + assert.Equal(big.NewInt(5), tx.GasTipCap()) + assert.Equal(big.NewInt(205), tx.GasFeeCap()) + assert.Nil(opts.GasTipCap) + assert.Nil(opts.GasFeeCap) + assert.True(mt.suggestGasTipCapCalled) + + // Second call to Transact should use latest suggested GasTipCap + mt.gasTipCap = big.NewInt(6) + mt.suggestGasTipCapCalled = false + tx, err = bc.Transact(opts, "") + assert.Nil(err) + assert.Equal(big.NewInt(6), tx.GasTipCap()) + assert.Equal(big.NewInt(206), tx.GasFeeCap()) + assert.True(mt.suggestGasTipCapCalled) + + // GasPrice + // When opts.GasPrice is nil + mt = &mockTransactor{gasPrice: big.NewInt(5)} + bc = bind.NewBoundContract(common.Address{}, abi.ABI{}, nil, mt, nil) + opts = &bind.TransactOpts{Signer: mockSign} + tx, err = bc.Transact(opts, "") + assert.Nil(err) + assert.Equal(big.NewInt(5), tx.GasPrice()) + assert.Nil(opts.GasPrice) + assert.True(mt.suggestGasPriceCalled) + + // Second call to Transact should use latest suggested GasPrice + mt.gasPrice = big.NewInt(6) + mt.suggestGasPriceCalled = false + tx, err = bc.Transact(opts, "") + assert.Nil(err) + assert.Equal(big.NewInt(6), tx.GasPrice()) + assert.True(mt.suggestGasPriceCalled) +} + func unpackAndCheck(t *testing.T, bc *bind.BoundContract, expected map[string]interface{}, mockLog types.Log) { received := make(map[string]interface{}) if err := bc.UnpackLogIntoMap(received, "received", mockLog); err != nil { From 405078065695a993b10447912599a4db91830fb0 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Tue, 12 Oct 2021 13:33:15 +0200 Subject: [PATCH 2/5] accounts/abi/bind: refactor transact method --- accounts/abi/bind/base.go | 207 ++++++++++++++++++++++---------------- 1 file changed, 123 insertions(+), 84 deletions(-) diff --git a/accounts/abi/bind/base.go b/accounts/abi/bind/base.go index 5add5de0d57e5..f8f9b21b93b02 100644 --- a/accounts/abi/bind/base.go +++ b/accounts/abi/bind/base.go @@ -231,111 +231,150 @@ func (c *BoundContract) Transfer(opts *TransactOpts) (*types.Transaction, error) return c.transact(opts, &c.address, nil) } -// transact executes an actual transaction invocation, first deriving any missing -// authorization fields, and then scheduling the transaction for execution. -func (c *BoundContract) transact(opts *TransactOpts, contract *common.Address, input []byte) (*types.Transaction, error) { - var err error - - // Ensure a valid value field and resolve the account nonce +func (c *BoundContract) createDynamicTx(opts *TransactOpts, contract *common.Address, input []byte, head *types.Header) (*types.Transaction, error) { + // Normalize value value := opts.Value if value == nil { value = new(big.Int) } - var nonce uint64 - if opts.Nonce == nil { - nonce, err = c.transactor.PendingNonceAt(ensureContext(opts.Context), opts.From) + // Estimate TipCap + gasTipCap := opts.GasTipCap + if gasTipCap == nil { + tip, err := c.transactor.SuggestGasTipCap(ensureContext(opts.Context)) if err != nil { - return nil, fmt.Errorf("failed to retrieve account nonce: %v", err) + return nil, err } - } else { - nonce = opts.Nonce.Uint64() + gasTipCap = tip } - - // Figure out reasonable gas price values - gasPrice := opts.GasPrice - gasTipCap := opts.GasTipCap + // Estimate FeeCap gasFeeCap := opts.GasFeeCap - if gasPrice != nil && (gasFeeCap != nil || gasTipCap != nil) { - return nil, errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified") + if gasFeeCap == nil { + gasFeeCap = new(big.Int).Add( + gasTipCap, + new(big.Int).Mul(head.BaseFee, big.NewInt(2)), + ) } - head, err := c.transactor.HeaderByNumber(ensureContext(opts.Context), nil) + if gasFeeCap.Cmp(gasTipCap) < 0 { + return nil, fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", gasFeeCap, gasTipCap) + } + // Estimate GasLimit + gasLimit := opts.GasLimit + if opts.GasLimit == 0 { + var err error + gasLimit, err = c.estimateGasLimit(opts, contract, input, nil, gasTipCap, gasFeeCap, value) + if err != nil { + return nil, err + } + } + // getNonce + nonce, err := c.getNonce(opts) if err != nil { return nil, err } - if head.BaseFee != nil && gasPrice == nil { - if gasTipCap == nil { - tip, err := c.transactor.SuggestGasTipCap(ensureContext(opts.Context)) - if err != nil { - return nil, err - } - gasTipCap = tip - } - if gasFeeCap == nil { - gasFeeCap = new(big.Int).Add( - gasTipCap, - new(big.Int).Mul(head.BaseFee, big.NewInt(2)), - ) - } - if gasFeeCap.Cmp(gasTipCap) < 0 { - return nil, fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", gasFeeCap, gasTipCap) - } - } else { - if gasFeeCap != nil || gasTipCap != nil { - return nil, errors.New("maxFeePerGas or maxPriorityFeePerGas specified but london is not active yet") - } - if gasPrice == nil { - price, err := c.transactor.SuggestGasPrice(ensureContext(opts.Context)) - if err != nil { - return nil, err - } - gasPrice = price + // create the transaction + baseTx := &types.DynamicFeeTx{ + Nonce: nonce, + GasFeeCap: gasFeeCap, + GasTipCap: gasTipCap, + Gas: gasLimit, + Value: value, + Data: input, + } + if contract != nil { + baseTx.To = &c.address + } + return types.NewTx(baseTx), nil +} + +func (c *BoundContract) createLegacyTx(opts *TransactOpts, contract *common.Address, input []byte) (*types.Transaction, error) { + if opts.GasFeeCap != nil || opts.GasTipCap != nil { + return nil, errors.New("maxFeePerGas or maxPriorityFeePerGas specified but london is not active yet") + } + // Normalize value + value := opts.Value + if value == nil { + value = new(big.Int) + } + // Estimate GasPrice + gasPrice := opts.GasPrice + if gasPrice == nil { + price, err := c.transactor.SuggestGasPrice(ensureContext(opts.Context)) + if err != nil { + return nil, err } + gasPrice = price } + // Estimate GasLimit gasLimit := opts.GasLimit - if gasLimit == 0 { - // Gas estimation cannot succeed without code for method invocations - if contract != nil { - if code, err := c.transactor.PendingCodeAt(ensureContext(opts.Context), c.address); err != nil { - return nil, err - } else if len(code) == 0 { - return nil, ErrNoCode - } - } - // If the contract surely has code (or code is not needed), estimate the transaction - msg := ethereum.CallMsg{From: opts.From, To: contract, GasPrice: gasPrice, GasTipCap: gasTipCap, GasFeeCap: gasFeeCap, Value: value, Data: input} - gasLimit, err = c.transactor.EstimateGas(ensureContext(opts.Context), msg) + if opts.GasLimit == 0 { + var err error + gasLimit, err = c.estimateGasLimit(opts, contract, input, gasPrice, nil, nil, value) if err != nil { - return nil, fmt.Errorf("failed to estimate gas needed: %v", err) + return nil, err } } - // Create the transaction, sign it and schedule it for execution - var rawTx *types.Transaction - if gasFeeCap == nil { - baseTx := &types.LegacyTx{ - Nonce: nonce, - GasPrice: gasPrice, - Gas: gasLimit, - Value: value, - Data: input, - } - if contract != nil { - baseTx.To = &c.address + // getNonce + nonce, err := c.getNonce(opts) + if err != nil { + return nil, err + } + // create the transaction + baseTx := &types.LegacyTx{ + Nonce: nonce, + GasPrice: gasPrice, + Gas: gasLimit, + Value: value, + Data: input, + } + if contract != nil { + baseTx.To = &c.address + } + return types.NewTx(baseTx), err +} + +func (c *BoundContract) estimateGasLimit(opts *TransactOpts, contract *common.Address, input []byte, gasPrice, gasTipCap, gasFeeCap, value *big.Int) (uint64, error) { + // Gas estimation cannot succeed without code for method invocations + if contract != nil { + if code, err := c.transactor.PendingCodeAt(ensureContext(opts.Context), c.address); err != nil { + return 0, err + } else if len(code) == 0 { + return 0, ErrNoCode } - rawTx = types.NewTx(baseTx) + } + // If the contract surely has code (or code is not needed), estimate the transaction + msg := ethereum.CallMsg{From: opts.From, To: contract, GasPrice: gasPrice, GasTipCap: gasTipCap, GasFeeCap: gasFeeCap, Value: value, Data: input} + return c.transactor.EstimateGas(ensureContext(opts.Context), msg) +} + +func (c *BoundContract) getNonce(opts *TransactOpts) (uint64, error) { + if opts.Nonce == nil { + return c.transactor.PendingNonceAt(ensureContext(opts.Context), opts.From) } else { - baseTx := &types.DynamicFeeTx{ - Nonce: nonce, - GasFeeCap: gasFeeCap, - GasTipCap: gasTipCap, - Gas: gasLimit, - Value: value, - Data: input, - } - if contract != nil { - baseTx.To = &c.address - } - rawTx = types.NewTx(baseTx) + return opts.Nonce.Uint64(), nil + } +} + +// transact executes an actual transaction invocation, first deriving any missing +// authorization fields, and then scheduling the transaction for execution. +func (c *BoundContract) transact(opts *TransactOpts, contract *common.Address, input []byte) (*types.Transaction, error) { + if opts.GasPrice != nil && (opts.GasFeeCap != nil || opts.GasTipCap != nil) { + return nil, errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified") + } + head, err := c.transactor.HeaderByNumber(ensureContext(opts.Context), nil) + if err != nil { + return nil, err + } + // Create the transaction + var rawTx *types.Transaction + if head.BaseFee != nil && opts.GasPrice == nil { + rawTx, err = c.createDynamicTx(opts, contract, input, head) + } else { + rawTx, err = c.createLegacyTx(opts, contract, input) + } + if err != nil { + return nil, err } + // Sign the transaction and schedule it for execution if opts.Signer == nil { return nil, errors.New("no signer to authorize the transaction with") } From e1f7cd03e923dff40992b4a8ebda3353b3e6b1bb Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Tue, 12 Oct 2021 14:17:29 +0200 Subject: [PATCH 3/5] accounts/abi/bind: only query header if no gas price is specified --- accounts/abi/bind/base.go | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/accounts/abi/bind/base.go b/accounts/abi/bind/base.go index f8f9b21b93b02..24baf2f949797 100644 --- a/accounts/abi/bind/base.go +++ b/accounts/abi/bind/base.go @@ -360,16 +360,23 @@ func (c *BoundContract) transact(opts *TransactOpts, contract *common.Address, i if opts.GasPrice != nil && (opts.GasFeeCap != nil || opts.GasTipCap != nil) { return nil, errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified") } - head, err := c.transactor.HeaderByNumber(ensureContext(opts.Context), nil) - if err != nil { - return nil, err - } // Create the transaction - var rawTx *types.Transaction - if head.BaseFee != nil && opts.GasPrice == nil { - rawTx, err = c.createDynamicTx(opts, contract, input, head) - } else { + var ( + rawTx *types.Transaction + err error + ) + if opts.GasPrice != nil { rawTx, err = c.createLegacyTx(opts, contract, input) + } else { + // Only query for basefee if gasPrice not specified + if head, errHead := c.transactor.HeaderByNumber(ensureContext(opts.Context), nil); err != nil { + return nil, errHead + } else if head.BaseFee != nil { + rawTx, err = c.createDynamicTx(opts, contract, input, head) + } else { + // Chain is not London ready -> use legacy transaction + rawTx, err = c.createLegacyTx(opts, contract, input) + } } if err != nil { return nil, err From f977f2cc6d7b11af348b9ccf2143d26ad61c087e Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Tue, 12 Oct 2021 16:31:48 +0200 Subject: [PATCH 4/5] accounts/abi/bind: improve comment --- accounts/abi/bind/base.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/accounts/abi/bind/base.go b/accounts/abi/bind/base.go index 24baf2f949797..a6927ecdd0a94 100644 --- a/accounts/abi/bind/base.go +++ b/accounts/abi/bind/base.go @@ -266,12 +266,11 @@ func (c *BoundContract) createDynamicTx(opts *TransactOpts, contract *common.Add return nil, err } } - // getNonce + // create the transaction nonce, err := c.getNonce(opts) if err != nil { return nil, err } - // create the transaction baseTx := &types.DynamicFeeTx{ Nonce: nonce, GasFeeCap: gasFeeCap, @@ -313,12 +312,11 @@ func (c *BoundContract) createLegacyTx(opts *TransactOpts, contract *common.Addr return nil, err } } - // getNonce + // create the transaction nonce, err := c.getNonce(opts) if err != nil { return nil, err } - // create the transaction baseTx := &types.LegacyTx{ Nonce: nonce, GasPrice: gasPrice, From 2cbe6ea52da717d99a1cfeeb5f7b092705d68735 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Wed, 13 Oct 2021 17:55:00 +0200 Subject: [PATCH 5/5] accounts/abi/bind: use contract address from parameter for tx.To --- accounts/abi/bind/base.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/accounts/abi/bind/base.go b/accounts/abi/bind/base.go index a6927ecdd0a94..63280bcbebcc6 100644 --- a/accounts/abi/bind/base.go +++ b/accounts/abi/bind/base.go @@ -272,6 +272,7 @@ func (c *BoundContract) createDynamicTx(opts *TransactOpts, contract *common.Add return nil, err } baseTx := &types.DynamicFeeTx{ + To: contract, Nonce: nonce, GasFeeCap: gasFeeCap, GasTipCap: gasTipCap, @@ -279,9 +280,6 @@ func (c *BoundContract) createDynamicTx(opts *TransactOpts, contract *common.Add Value: value, Data: input, } - if contract != nil { - baseTx.To = &c.address - } return types.NewTx(baseTx), nil } @@ -318,29 +316,34 @@ func (c *BoundContract) createLegacyTx(opts *TransactOpts, contract *common.Addr return nil, err } baseTx := &types.LegacyTx{ + To: contract, Nonce: nonce, GasPrice: gasPrice, Gas: gasLimit, Value: value, Data: input, } - if contract != nil { - baseTx.To = &c.address - } - return types.NewTx(baseTx), err + return types.NewTx(baseTx), nil } func (c *BoundContract) estimateGasLimit(opts *TransactOpts, contract *common.Address, input []byte, gasPrice, gasTipCap, gasFeeCap, value *big.Int) (uint64, error) { - // Gas estimation cannot succeed without code for method invocations if contract != nil { + // Gas estimation cannot succeed without code for method invocations. if code, err := c.transactor.PendingCodeAt(ensureContext(opts.Context), c.address); err != nil { return 0, err } else if len(code) == 0 { return 0, ErrNoCode } } - // If the contract surely has code (or code is not needed), estimate the transaction - msg := ethereum.CallMsg{From: opts.From, To: contract, GasPrice: gasPrice, GasTipCap: gasTipCap, GasFeeCap: gasFeeCap, Value: value, Data: input} + msg := ethereum.CallMsg{ + From: opts.From, + To: contract, + GasPrice: gasPrice, + GasTipCap: gasTipCap, + GasFeeCap: gasFeeCap, + Value: value, + Data: input, + } return c.transactor.EstimateGas(ensureContext(opts.Context), msg) }