feat: eip 1559 dynamic transactions #3504
Conversation
pkg/transaction/transaction.go
Outdated
| Value: request.Value, | ||
| Gas: gasLimit, | ||
| GasTipCap: gasPrice, | ||
| GasFeeCap: gasPrice, |
There was a problem hiding this comment.
they both use the same value?
There was a problem hiding this comment.
yes, I've added some comments that clarifies it hopefully
pkg/transaction/transaction.go
Outdated
| return nil, ErrGasPriceTooLow | ||
| } | ||
| gasPrice = new(big.Int).Div(new(big.Int).Mul(big.NewInt(int64(tipPercent)+100), gasPrice), big.NewInt(100)) |
There was a problem hiding this comment.
Wait, you're going to boost the provided price? Shouldn't the boost be inside the gasPrice == nil block? If the user specifies a price, then I don't think they'd expect it to be boosted before being used.
| @@ -254,7 +254,7 @@ func (t *transactionService) StoredTransaction(txHash common.Hash) (*StoredTrans | |||
| } | |||
|
|
|||
| // prepareTransaction creates a signable transaction based on a request. | |||
| func (t *transactionService) prepareTransaction(ctx context.Context, request *TxRequest, nonce uint64, boostPercent int) (tx *types.Transaction, err error) { | |||
| func (t *transactionService) prepareTransaction(ctx context.Context, request *TxRequest, nonce uint64, tipPercent int) (tx *types.Transaction, err error) { | |||
There was a problem hiding this comment.
Would this be better as tipBoostPercent?
| Value: storedTransaction.Value, | ||
| Gas: storedTransaction.GasLimit, | ||
| GasTipCap: storedTransaction.GasPrice, | ||
| GasFeeCap: storedTransaction.GasPrice, |
There was a problem hiding this comment.
Shouldn't we be recalling the individual GasTipCap and GasFeeCap that were originally specified for the transaction? Or haven't we modified the transaction store for the new fields yet?
There was a problem hiding this comment.
it's ok to put the gas price of the transaction as the gas fee cap and tip because the gas price was originally calculated using the gas tip cap and gas fee cap.
here is an example:
https://goerli.etherscan.io/tx/0x621afbab180977bc77230ec1d5066bda0c617bd8a7126f4896c384ffb8b94dc7
where the gas price is base fee + gas tip, with an upper bound of gas fee cap.
ideally, what needs to be done is get a fresh suggested gas price and tip cap and resend the trx with that because the base price of the blockchain might have changed in the meanwhile.
| Value: big.NewInt(0), | ||
| Gas: 21000, | ||
| GasTipCap: gasPrice, | ||
| GasFeeCap: gasPrice, |
There was a problem hiding this comment.
Ditto here on the difference between GasTipCap and GasFeeCap?
There was a problem hiding this comment.
we have a test case called "sendWithBoost" which defines a different tip and fee, this is sufficient unit testing for now.
| return signermock.New( | ||
| signermock.WithSignTxFunc(func(transaction *types.Transaction, chainID *big.Int) (*types.Transaction, error) { | ||
| if transaction.Type() != 0 { | ||
| if transaction.Type() != 2 { | ||
| t.Fatalf("wrong transaction type. wanted 0, got %d", transaction.Type()) | ||
| } |
There was a problem hiding this comment.
Waring should be "wanted 2", not "wanted 0".
| Value: value, | ||
| Gas: estimatedGasLimit, | ||
| GasTipCap: suggestedGasPrice, | ||
| GasFeeCap: suggestedGasPrice, |
There was a problem hiding this comment.
For the test, do we want to separate the GasTipCap and GasFeeCap to 2 different values to ensure that they are properly carried through and not mixed?
| Value: value, | ||
| Gas: estimatedGasLimit, | ||
| GasTipCap: suggestedGasPrice, | ||
| GasFeeCap: suggestedGasPrice, |
There was a problem hiding this comment.
Ditto with the different values for traceability.
| @@ -445,6 +466,9 @@ func TestTransactionSend(t *testing.T) { | |||
| backendmock.WithPendingNonceAtFunc(func(ctx context.Context, account common.Address) (uint64, error) { | |||
| return nextNonce, nil | |||
| }), | |||
| backendmock.WithSuggestGasTipCapFunc(func(ctx context.Context) (*big.Int, error) { | |||
| return big.NewInt(0), nil | |||
There was a problem hiding this comment.
Ditto with returning a uniquely recognizable value.
| Value: value, | ||
| Gas: gasLimit, | ||
| GasTipCap: gasPrice, | ||
| GasFeeCap: gasPrice, |
There was a problem hiding this comment.
Ditto with different values for Tip and Fee.
| Value: value, | ||
| Gas: gasLimit, | ||
| GasTipCap: gasPrice, | ||
| GasFeeCap: gasPrice, |
There was a problem hiding this comment.
And again (hopefully this is the last instance of this!).
| Value: big.NewInt(0), | ||
| Gas: 21000, | ||
| GasTipCap: price, | ||
| GasFeeCap: price, |
There was a problem hiding this comment.
It wasn't the last, and neither is this, but I'll leave it to you to apply the suggestion to all tests or none.
This reverts commit e11d96b.
Checklist
Description
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
This change is