chore: add EIP-1559 implementation for gas price#113
Conversation
emizzle
left a comment
emizzle
left a comment
There was a problem hiding this comment.
Generally LGTM. Should we include:
- Validation of
maxPriorityFeePerGas. Maybe a warning if it's too low (< 500k) that the fee is low and it reduces the chances of the transaction being included in the block - Tests on bounds of
maxPriorityFeePerGas, so that we know what bounds are acceptable for users.
…ll transactions are using EIP1559 by default
2-towns
force-pushed
the
feat/implements-EIP-1559-gas-calculation
branch
from
e80880e to
d4fcb37
Compare
…yFeePerGas as a fallback
| trace "EIP-1559 is supported", maxPriorityFeePerGas = maxPriorityFeePerGas, maxFeePerGas = maxFeePerGas | ||
| else: | ||
| populated.gasPrice = some(transaction.gasPrice |? (await signer.getGasPrice())) | ||
| trace "EIP-1559 is not supported", gasPrice = populated.gasPrice |
There was a problem hiding this comment.
Should we set populated.maxFeePerGas = none(UInt256) just in case here? Thinking of a case where the transaction.maxFeePerGas was already populated but the block didn't have a baseFeePerGas value.
There was a problem hiding this comment.
I added it just in case, but it should not have any impact. If the block doesn't have baseFeePerGas that means EIP-1559 is not supported so maxFeePerGas will not be used.
There was a problem hiding this comment.
Fair point. When the transaction is signed in the wallet, the first condition checked is whether or not transaction.maxFeePerGas has a value (see line 35 in signers/wallet/signing.nim), which ends up setting signable.txType = TxEip1559 and could have side effects later on.
There was a problem hiding this comment.
We may also want to set maxPriorityFeePerGas to none just in case. Just a suggestion.
There was a problem hiding this comment.
Yes you are right. I just updated the PR to set none to maxPriorityFeePerGas to avoid side effect. Thanks.
…void potential side effect in wallet signing
3 participants
No description provided.