-
-
Notifications
You must be signed in to change notification settings - Fork 109
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
Free Gas Tracker Endpoints #74
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, again!
The unit tests currently just check for existence of a value and no error for the Gas Tracker methods since each call generates dynamic data. Is there a way to test with static data?
Personally I think it's good enough. The value is dynamic, so any value >= 0 is good.
gas_tracker.go
Outdated
package etherscan | ||
|
||
// GasEstiamte gets estiamted confirmation time (in seconds) at the given gas price | ||
func (c *Client) GasEstimate(gasPrice int) (confirmationTimeInSec string, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should make confirmationTimeInSec
a int
? An user of this method may find it handy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An int
is good, a time.Duration
is better IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, let's use time.Duration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 384329f
response.go
Outdated
//Gas Prices are returned in Gwei | ||
type GasPrices struct { | ||
LastBlock string | ||
SafeGasPrice string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since gas price is in Gwei, maybe we should use a float64
?
I am curious, what data type are you using when you consume GasPrices
struct? We may use the type dirctly so that our user don't have to do the convertion by themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having LastBlock
be an int and converting the prices to either float64
or decimal.Decimal
is better. I personally would consume the Gas prices as decimal.Decimal
, however, I understand not going that route since the idea behind this API is to leverage the go standard library.
I left it as a string
since that's what the API returns, however, I'll looking into converting into a numeric type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 384329f
7137647
to
384329f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for your contribution!
Implementation for Free Gas Tracker Endpoints (partial implementation of Issue #70).
I can use suggestions for:
1.) Values returned: Etherscan returns everything as a string so the methods currently return everything as a string as well. I'm open to writing custom JSON Unmarshal methods to convert the comma separated strings into slices and convert numeric string values into float64/decimal.Decimal.
2.) The unit tests currently just check for existence of a value and no error for the Gas Tracker methods since each call generates dynamic data. Is there a way to test with static data?