feat: eip-1559 fixed gas prices #2398
Conversation
Fixes the evaluation of the chain's gas configs
Branch preview✅ Deploy successful! https://feat_fixed_1559_gasprice--walletweb.review-wallet-web.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
| await act(async () => { | ||
| await Promise.resolve() | ||
| }) | ||
| // assert the hook is not loading | ||
| expect(result.current[2]).toBe(false) | ||
|
|
||
| // assert fixed gas price as minimum of 0.1 gwei | ||
| expect(result.current[0]?.maxFeePerGas?.toString()).toBe('100000000') | ||
|
|
||
| // assert fixed priority fee | ||
| expect(result.current[0]?.maxPriorityFeePerGas?.toString()).toBe('100000') |
There was a problem hiding this comment.
nit: You can also wrap all the expects with an await waitFor() call instead of having an empty Promise.resolve() to achieve the same
| const getGasParameters = ( | ||
| estimation: EstimatedGasPrice | undefined, | ||
| feeData: FeeData | undefined, | ||
| isEIP1559: boolean, | ||
| ): GasFeeParams => { | ||
| if (!estimation) { | ||
| return { | ||
| maxFeePerGas: isEIP1559 ? feeData?.maxFeePerGas : feeData?.gasPrice, | ||
| maxPriorityFeePerGas: isEIP1559 ? feeData?.maxPriorityFeePerGas : undefined, | ||
| } | ||
| } | ||
|
|
||
| if (isEIP1559 && 'maxFeePerGas' in estimation && 'maxPriorityFeePerGas' in estimation) { | ||
| return estimation | ||
| } | ||
|
|
||
| if ('gasPrice' in estimation) { | ||
| return { | ||
| maxFeePerGas: estimation.gasPrice, | ||
| maxPriorityFeePerGas: isEIP1559 ? feeData?.maxPriorityFeePerGas : undefined, | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| maxFeePerGas: undefined, | ||
| maxPriorityFeePerGas: undefined, | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks more readable now but I would suggest to write a few tests to cover some of the edge-cases like if there is no estimation and it is EIP1559 but there is also no feeData etc.
|
For goerli I think 200 is waaay to high. Not a big deal since goerli is a test network, but I think we can reduce it to 50 or so: |
|
Tried send funds for ETH and Poly and Add owner for Optimism In optimism the values are set to 0.1 for maxFeePerGas and 0.0001 for maxPriorityFeePerGas and the tx works fine. These values were suggested by the people on optimism just fine For ETH the tx worked fine, the values passed to MM are correct For Poly we is also working fine, takes about 2 or so mins for the tx to go through tho. We are estimating a value that is slower than the "Slow" MM estimation, so we should monitor the user experience after releasing this. NOTE: we seen with Manu and Usame that in Poly MM is using the same number of Prio Fee and uses it into Max base fee as well. It seems is the way it works for poly and it is how it works in Prod currently, so not an issue, but still leaving it here for the Since executing the tx works, and the only "issue" that I had was goerli way overestimating the gasprice I'm fine passing this ticket. |
|
Hey @francovenica |
|
Passing it then. thanks for the comment |
What it solves
This PR fixes the usage of chain gas configs in general and adds a new fixed gas config.
Resolves #2222
How this PR fixes it
Enabled fixed gas prices for EIP-1559
How to test it
Fixed 1559 gas prices
We added Optimism with the desired gas price config to staging and enabled 1559 there.
Bug fix: Use gas price configs
This PR also fixed our gas estimations in general which never considered the gas price configs from our config service before.
To test this try to create transactions on networks with specific gas price configs:
Screenshots
The screenshot shows the desired fixed gas price of 0.1 maxFee and 0.0001 maxPrio on Optimism.
Checklist