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

graphql: return decimal for estimateGas and cumulativeGas queries #22126

Merged
merged 3 commits into from
Jan 6, 2021

Conversation

renaynay
Copy link
Contributor

@renaynay renaynay commented Jan 6, 2021

This PR updates the return values for estimateGas and cumulativeGas to decimal rather than hexadecimal, as defined by the spec.

@renaynay renaynay marked this pull request as ready for review January 6, 2021 15:19
@renaynay renaynay requested a review from gballet as a code owner January 6, 2021 15:19
Copy link
Contributor

@decanus decanus left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@tuxxy tuxxy left a comment

Choose a reason for hiding this comment

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

🐧

@renaynay renaynay requested a review from fjl January 6, 2021 15:30
Copy link

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

Copy link

@mirshko mirshko left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -811,15 +811,15 @@ type CallData struct {
// CallResult encapsulates the result of an invocation of the `call` accessor.
type CallResult struct {
data hexutil.Bytes // The return data from the call
gasUsed hexutil.Uint64 // The amount of gas used
gasUsed Long // The amount of gas used
Copy link

Choose a reason for hiding this comment

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

Long what?

Choose a reason for hiding this comment

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

Either a typo or type alias?

Copy link
Contributor

Choose a reason for hiding this comment

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

LONG $ETH

Copy link
Contributor

Choose a reason for hiding this comment

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

long eth, short tron

Copy link

Choose a reason for hiding this comment

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

Is this financial advice?

Copy link

Choose a reason for hiding this comment

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

gas is too long to pass

Copy link
Contributor

@zscole zscole left a comment

Choose a reason for hiding this comment

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

Please conform to this format in the future.

@@ -301,21 +301,21 @@ func (t *Transaction) Status(ctx context.Context) (*hexutil.Uint64, error) {
return &ret, nil
}

func (t *Transaction) GasUsed(ctx context.Context) (*hexutil.Uint64, error) {
func (t *Transaction) GasUsed(ctx context.Context) (*Long, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

too long

receipt, err := t.getReceipt(ctx)
if err != nil || receipt == nil {
return nil, err
}
ret := hexutil.Uint64(receipt.GasUsed)
ret := Long(receipt.GasUsed)
Copy link
Contributor

Choose a reason for hiding this comment

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

way too long

receipt, err := t.getReceipt(ctx)
if err != nil || receipt == nil {
return nil, err
}
ret := hexutil.Uint64(receipt.CumulativeGasUsed)
ret := Long(receipt.CumulativeGasUsed)
Copy link
Contributor

Choose a reason for hiding this comment

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

man, this is really long

@@ -811,15 +811,15 @@ type CallData struct {
// CallResult encapsulates the result of an invocation of the `call` accessor.
type CallResult struct {
data hexutil.Bytes // The return data from the call
gasUsed hexutil.Uint64 // The amount of gas used
gasUsed Long // The amount of gas used
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd like to speak to the manager

@@ -811,15 +811,15 @@ type CallData struct {
// CallResult encapsulates the result of an invocation of the `call` accessor.
type CallResult struct {
data hexutil.Bytes // The return data from the call
gasUsed hexutil.Uint64 // The amount of gas used
gasUsed Long // The amount of gas used
Copy link
Contributor

Choose a reason for hiding this comment

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

long eth, short tron

@@ -117,6 +117,12 @@ func TestGraphQLBlockSerialization(t *testing.T) {
want: `{"errors":[{"message":"Cannot query field \"bleh\" on type \"Query\".","locations":[{"line":1,"column":2}]}]}`,
code: 400,
},
// should return `estimateGas` as decimal
{
body: `{"query": "{block{ estimateGas(data:{}) }}"}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

peer pressure

@gballet gballet added this to the 1.10.0 milestone Jan 6, 2021
@gballet gballet merged commit 072fd96 into ethereum:master Jan 6, 2021
@renaynay renaynay deleted the graphql_gas_queries branch January 6, 2021 16:20
@karalabe
Copy link
Member

karalabe commented Jan 6, 2021

Woah, I didn't evne comment and this got merged. Gonna have to revert, sorry.

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.