Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitized provider recommended gas price #1772

Closed
wants to merge 9 commits into from

Conversation

cserb
Copy link
Contributor

@cserb cserb commented May 17, 2023

Closes #1711

@cserb
Copy link
Contributor Author

cserb commented May 17, 2023

@bdrhn9 before I continue and also adapt the test, can you confirm this is going in the right direction

@cserb cserb requested a review from bdrhn9 May 17, 2023 13:26
@bdrhn9
Copy link
Contributor

bdrhn9 commented May 17, 2023

Nit again: alternative naming for baseFeePerGas could be baseFee. This leads to shorten all variables include baseFeePerGas like:

baseFeePerGasMultiplier => baseFeeMultiplier
baseFeePerGasMultiplierThreshold => baseFeeMultiplierThreshold
baseFeeTip => additionalGasFee 😋

However this suggestion more than nit :-D so feel free to ignore.

@cserb
Copy link
Contributor Author

cserb commented May 18, 2023

Nit again: alternative naming for baseFeePerGas could be baseFee. This leads to shorten all variables include baseFeePerGas like:

baseFeePerGasMultiplier => baseFeeMultiplier baseFeePerGasMultiplierThreshold => baseFeeMultiplierThreshold baseFeeTip => additionalGasFee 😋

However this suggestion more than nit :-D so feel free to ignore.

Well maybe not even so much of a nit. I've only now seen that we are using everywhere else baseFee and what I called baseFeeTip is priorityTip for consistency I will change it.

@cserb cserb marked this pull request as ready for review May 19, 2023 12:03
@cserb
Copy link
Contributor Author

cserb commented May 19, 2023

Final review @bdrhn9 😄 ?

@cserb cserb requested a review from bdrhn9 May 19, 2023 12:04
@aquarat
Copy link
Contributor

aquarat commented Jun 23, 2023

Pending manual testing.

export const fetchBaseFeePerGas = async (
provider: Provider,
startTime: number
): Promise<ethers.providers.Block['baseFeePerGas']> => {
const goLatestBlock = await go(() => provider.getBlock('latest'), {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function throws error when block header couldn't be fetched. In this case, gasOracle tries to the following strategy if it is defined. In case it's defined, it'll be probably providerRecommendedGasPrice strategy and again we make another RPC request to get providerRecommendedGasPrice. Or the other case where there is no next strategy defined, gasOracle will use constant gas price which is not safe for all chain. So instead of throwing error when block header couldn't be fetched, returning already fetched and calculated providerRecommendedGasPrice is more efficient and safe way to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better and more elegant solution is first making RPC call for block header (throws error if fail) and then making RPC call for providerRecommendedGasPrice. So in this case, if block header can't be fetched, gasOracle tries the next strategy which will be probably providerRecommendedGasPrice.

@bdrhn9 bdrhn9 self-assigned this Jul 6, 2023
@bdrhn9 bdrhn9 removed their assignment Jul 9, 2023
@bdrhn9
Copy link
Contributor

bdrhn9 commented Jul 9, 2023

Because new implementation and testing is necessary, it's better to continue on new PR #1828.

@bdrhn9 bdrhn9 closed this Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the gas price strategy: Sanitized providerRecommendedGasPrice
4 participants