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

Add option to use ethgasstation.info for gas price estimation #1

Closed
wants to merge 16 commits into from
Closed

Add option to use ethgasstation.info for gas price estimation #1

wants to merge 16 commits into from

Conversation

alexandre-abrioux
Copy link
Contributor

@alexandre-abrioux alexandre-abrioux commented Jan 30, 2021

Hi!

First of all, thanks to the team for creating and developing this amazing project.

I've been running a noether node for almost a month now and I've suffered several failed transaction due to high volatility/fluctuation of Ethereum gas price.

During those hiccups the gas price set by Infura would be too low for transactions to be processed quickly enough by Ethereum network to fit in the block producing eligibility window.

Some transactions (not linked for privacy but can do through private channel if needed) would take up to an hour to get validated by Ethereum network and, once approved, would be refused by the contract for producing new blocks (as intended as they were delayed too much).

The solution I am bringing to the code base is to fetch the gas price from ethgasstation.info before sending the transaction. I tested it for several weeks now and found the best way to ensure no transaction fails is to use the fast gas price provided by the service, as I got failed transactions with both the cheap and average gas price profiles.

The API URL is configurable (GAS_STATION_API_URL) so that a similar services or privately hosted stations could be used.

A timeout (GAS_STATION_API_REQUEST_TIMEOUT_MS), set to 10 seconds by default, ensures that the node produces new blocks in case the API becomes unreachable.

The whole "fetch gas from gas station" feature can also be disabled in the config (GAS_STATION_API_ENABLED).

The GAS_MULTIPLIER config has been split into two different constants:

  • GAS_LIMIT_MULTIPLIER
  • and GAS_PRICE_MULTIPLIER

; so that the gas price multiplier could be configured independently from the gas limit multiplier.

The gas price multiplier is used when the gas station feature is disabled or when the API is unreachable.

The gas station profile (fast by default) is also configurable (GAS_STATION_API_PROFILE).

The gas price is fetched only for the GAS_STATION_API_CHAIN_ID (1 by default). So for other chains it uses Infura gas price along with the GAS_PRICE_MULTIPLIER.

The developped code is tested with mocha so I had to add some new dev dependencies:

  • mocha, chai, sinon for testing ;
  • moxios to easily mock axios ;
  • waffle to mock the provider.

I had to add git in the Dockerfile because some dependencies of the new packages can only be fetched through git.

I also added a tsconfig.prod.json to prevent building test files in the production build.

If this PR gets accepted we could in the future add those tests to the CI pipeline.

I hope my explanations were clear enough, please tell me if you have any question or you would like to see more improvement to the work I've done. Thanks!

@tuler
Copy link
Member

tuler commented Feb 1, 2021

Hello @alexandre-abrioux ! Thank you very much for your PR.
A better gas price estimator is a really nice addition to noether.

Testing with mocha is very welcome too.

The basic algorithm of using the fast predictor, and failing back to the provider gas is good.
I'd like to suggest a few changes to the code structure, which I believe will help maintenance in the future.

Consider the following idea:

  1. create a typescript interface called GasPriceProvider.
interface GasPriceProvider {
    getGasPrice(): Promise<BigNumber>;
}
  1. create a class implementation of that interface called ProviderGasPriceProvider. Its constructor receives a ethers provider and the multiplication factor.

  2. create an alternate class implementation called GasStationGasPriceProvider that does the job of talking to the gas station API.

  3. create a class implementation that tries one GasPriceProvider and falls back to another GasPriceProvider in case the first one fails, and wire the two together.

Let me know what you think about this design, and if you need more clarification or help with the code.

Thanks again

@alexandre-abrioux
Copy link
Contributor Author

Hello @tuler, thanks for your review, this makes sense! I'll find some time during the week to try and implement those changes and I'll get back to you :)

@alexandre-abrioux
Copy link
Contributor Author

@tuler Just finished implementing the requested changes, it feels much cleaner now! Tell me what you think about it.
I'm testing the new code on my production node, did not get any error so far. I'm gonna wait for a few blocks to get produced and I will get back to you if needed :)

Copy link
Member

@tuler tuler left a comment

Choose a reason for hiding this comment

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

It looks great! Thanks for the changes.

Can you include the copyright header to all added files?

src/config.ts Outdated Show resolved Hide resolved
src/config.ts Show resolved Hide resolved
@alexandre-abrioux
Copy link
Contributor Author

Added the copyright headers!

src/block.ts Outdated
const nonce = pos.signer.getTransactionCount("latest");
const gasPriceProvider = createGasPriceProvider(
pos.provider,
chainId
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a confusion here. The chainId in this context is the sidechain chainId, which starts with 0 and goes on as we can create additional chains in the future (for several reasons). It is not the Ethereum chain (mainnet, goerli, Rinkeby, etc).

It's our fault we are using the name chainId and causing this confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the mix-up, I just fixed that in a new commit. I kept the GAS_STATION_API_CHAIN_ID config that I introduced, I think it is still relevant, but it is now checked against the provider chainId in the createGasPriceProvider function.

@tuler
Copy link
Member

tuler commented Feb 8, 2021

Hello @alexandre-abrioux , as described in our contributing guidelines I'd like to ask you to sign the contributing agreement that is there and send over to [email protected].

Thank you again!

@alexandre-abrioux
Copy link
Contributor Author

@tuler I've filled up the Google Form "Enter E-mail Below to Request DocuSign". Is that OK? I haven't received anything yet.

@tuler
Copy link
Member

tuler commented Feb 11, 2021

@tuler I've filled up the Google Form "Enter E-mail Below to Request DocuSign". Is that OK? I haven't received anything yet.

That's it. We'll send you the doc soon. Thank you!

@alexandre-abrioux
Copy link
Contributor Author

OK thanks :)

@alexandre-abrioux
Copy link
Contributor Author

Hi again! I don't know if you got back the signed document from DocuSign, please tell me if I can help.

@tuler
Copy link
Member

tuler commented Feb 23, 2021

Hey @alexandre-abrioux , we got it, thanks!
We are doing final tests to prepare the release.

@tuler
Copy link
Member

tuler commented Mar 2, 2021

Your contribution has been integrated to the codebase through a rebase along with other changes.
I'll close this PR (instead of merging it).
Thank you again.

@tuler tuler closed this Mar 2, 2021
@alexandre-abrioux
Copy link
Contributor Author

Nice! Thanks.

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.

2 participants