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

Enhance gas price #803

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

odyslam
Copy link
Contributor

@odyslam odyslam commented Sep 20, 2021

image

Description

  • Make gas prices more human friendly
  • Show the user both the market price and the provider's price for gas
  • Color for the provider's price is adjusted based on market price, so the user can visually see the difference.

possible improvements

  • Take arguments to print only provider or only market view
  • Take arguments to export prices in a manner that can be consumed by other commands (e.g seth estimate to show size + deployment cost)

Very open to suggestions and feedback :)

@d-xo
Copy link
Contributor

d-xo commented Sep 21, 2021

I think this breaks both seth call and seth mktx which both use seth gas-price internally. It will also break any existing scripts that rely on the current output of seth gas-price.

I'm probably OK with breaking scripts that rely on seth gas-price since using gasnow data is a nice UX improvement, but we definately need to:

  • introduce some machine readable options into the output (maybe smth like: seth gas-price rapid, seth gas-price fast, seth gas-price standard, seth gas-price slow, seth gas-price rpc?). It might also be nice to introduce some SETH_TX_SPEED variable or similar that can be used to set a default?
  • fix the usages of seth gas-price in the existing codebase to avoid any regressions
  • add some automated tests covering the new behaviour

@odyslam
Copy link
Contributor Author

odyslam commented Sep 21, 2021

Thanks for the thorough response @d-xo. My main intention was to start a discussion, so I think that this is great.

For some reason, I didn't find any uses for the command. Weird.

These are the TODOs as I see them:

  • Introduce different commands that are machine-readable. For this, I prefer to concatenate in 2 commands that instead of returning a value, they define an env variable. market and provider. The reason being that a curl will bring the entire market view. Env variable: SETH_TX_FAST_GAS, etc.
  • Fix use of seth gas-price in existing
  • Add automated tests

What are you thinking about surfacing the following functionality:

As a user I want to know how much it will cost me to deploy my smart contract

This is a combination of seth estimate and seth gas-price. I have implemented it in this repo as an extension of @gakonst deployment script. It could be part of dapp test or another command.

@d-xo
Copy link
Contributor

d-xo commented Sep 21, 2021

For this, I prefer to concatenate in 2 commands that instead of returning a value, they define an env variable. market and provider. The reason being that a curl will bring the entire market view. Env variable: SETH_TX_FAST_GAS, etc.

I'm not sure I understand, can you provide some concrete examples.

What are you thinking about surfacing the following functionality:

As a user I want to know how much it will cost me to deploy my smart contract

This is a combination of seth estimate and seth gas-price. I have implemented it in this repo as an extension of @gakonst deployment script. It could be part of dapp test or another command.

I think this is a nice idea, but I think it's a little too specific to belong in core dapp. I think it makes more sense as a seperate tool.

@odyslam
Copy link
Contributor Author

odyslam commented Sep 21, 2021

I'm not sure I understand, can you provide some concrete examples.

  • seth gas-price market: Populate $ETH_TX_GAS_FAST, $ETH_TX_GAS_RAPID, etc
  • seth gas-price provider: Populate $ETH_TX_GAS_PROVIDER

I think this is a nice idea, but I think it's a little to specific to belong in core dapp. I think it makes more sense as a separate tool.

Is it too specific? I mean, if you are deploying a smart contract, you do want to know when it costs 1 ETH and when 3 ETH to deploy it, right? It could even be automated with a chron job so that it's deployed at the gas-price that you want.

At least, that was how I approached it. Since I am super new, I am very curious to see how teams approach this issue, or perhaps they don't care about deployment cost? Making this an issue for the hobbyist, but not the professional in a team.

@d-xo
Copy link
Contributor

d-xo commented Sep 21, 2021

  • seth gas-price market: Populate $ETH_TX_GAS_FAST, $ETH_TX_GAS_RAPID, etc
  • seth gas-price provider: Populate $ETH_TX_GAS_PROVIDER

I think this feels a little stateful and somewhat hard to compose. I would prefer keeping one command and allowing users to specify which output they want (e.g. seth gas-price rapid).

Is it too specific? I mean, if you are deploying a smart contract, you do want to know when it costs 1 ETH and when 3 ETH to deploy it, right? It could even be automated with a chron job so that it's deployed at the gas-price that you want.

At least, that was how I approached it. Since I am super new, I am very curious to see how teams approach this issue, or perhaps they don't care about deployment cost? Making this an issue for the hobbyist, but not the professional in a team.

I think you['ve changed my mind. A dapp create --estimate-cost or similar would be pretty nice actually.

@odyslam
Copy link
Contributor Author

odyslam commented Sep 21, 2021

I highly respect those who can change their mind. I hope it's as useful as I think it will be.

@transmissions11
Copy link
Contributor

transmissions11 commented Sep 30, 2021

oof GasNow is shutting down on October 18th so need to find another gas price api

@odyslam
Copy link
Contributor Author

odyslam commented Sep 30, 2021

Yeah, I need to get around to implementing this. Perhaps: https://www.blocknative.com/

@transmissions11
Copy link
Contributor

transmissions11 commented Sep 30, 2021

how about ethgasstation? https://ethgasstation.info/api/ethgasAPI.json

says they require an API key but works fine for me without

@mds1
Copy link
Contributor

mds1 commented Oct 6, 2021

+1 for blocknative's gas estimator, they're always great in my experience

@odyslam
Copy link
Contributor Author

odyslam commented Oct 7, 2021

Okay, I think I i will implement this the following days. It seems like an easy win.

I have no experience with either operator. What do you think is best? From a UX standpoint, ethgas is better because the user doesn't have to get a personal API key.

cc @mds1 @transmissions11 @d-xo

@transmissions11
Copy link
Contributor

transmissions11 commented Oct 7, 2021

Okay, I think I i will implement this the following days. It seems like an easy win.

I have no experience with either operator. What do you think is best? From a UX standpoint, ethgas is better because the user doesn't have to get a personal API key.

i def think we should find a free endpoint if possible— tho not sure which we can rely on

@sambacha suggested using api.txprice.com which he says is a free proxy they run for blocknative enterprise (foundry-rs/forge-template#24)

@mds1
Copy link
Contributor

mds1 commented Oct 7, 2021

Agreed on a free one, good point about the blocknative API key. Just tested/confirmed that https://api.txprice.com/ returns the same data as blocknative, so that seems to be a great option and will personally that will be my default

A good overview of other options can be found at https://github.com/sambacha/gas-reporting, also courtesy of @sambacha

@gakonst
Copy link
Contributor

gakonst commented Oct 7, 2021

Yeah +1 on free option

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.

5 participants