Skip to content

all: rename internal 1559 gas fields, add support for graphql#23010

Merged
holiman merged 2 commits intoethereum:masterfrom
karalabe:1559-graphql
Jun 8, 2021
Merged

all: rename internal 1559 gas fields, add support for graphql#23010
holiman merged 2 commits intoethereum:masterfrom
karalabe:1559-graphql

Conversation

@karalabe
Copy link
Copy Markdown
Member

@karalabe karalabe commented Jun 7, 2021

No description provided.

@holiman
Copy link
Copy Markdown
Contributor

holiman commented Jun 7, 2021

So, this changes feeCap to gasFeeCap, and tip to `gasTipCap.

	GasFeeCap        *hexutil.Big      `json:"maxFeePerGas,omitempty"`
	GasTipCap        *hexutil.Big      `json:"maxPriorityFeePerGas,omitempty"`

I agree it's a step forward, and think it's better than what we have now -- but I would personally prefer MaxFeePerGas and MaxPriorityFeePerGas, and live with the longer names.

Comment thread cmd/evm/testdata/10/txs.json Outdated
Comment on lines +14 to +15
"gasFeeCap" : "0xfa0",
"gasTipCap" : "0x0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess these should really map the actual json names, rather than what we use internally

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Where are these used?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When you run what's in the readme for that folder. But I just checked, it works fine now!

Comment thread core/state_processor_test.go Outdated
mkDynamicTx(0, common.Address{}, params.TxGas, big.NewInt(0), big.NewInt(0)),
},
want: "could not apply tx 0 [0xc4ab868fef0c82ae0387b742aee87907f2d0fc528fc6ea0a021459fb0fc4a4a8]: fee cap less than block base fee: address 0x71562b71999873DB5b286dF957af199Ec94617F7, feeCap: 0 baseFee: 875000000",
want: "could not apply tx 0 [0xc4ab868fef0c82ae0387b742aee87907f2d0fc528fc6ea0a021459fb0fc4a4a8]: fee cap less than block base fee: address 0x71562b71999873DB5b286dF957af199Ec94617F7, gasFeeCap: 0 baseFee: 875000000",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe these also should use the 'public' names, not the internal

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed.

@karalabe
Copy link
Copy Markdown
Member Author

karalabe commented Jun 8, 2021

I agree it's a step forward, and think it's better than what we have now -- but I would personally prefer MaxFeePerGas and MaxPriorityFeePerGas, and live with the longer names.

I'm kind of against that. It's important to keep the codebase consistent.

Copy link
Copy Markdown
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman merged commit c503f98 into ethereum:master Jun 8, 2021
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
…um#23010)

* all: rename internal 1559 gas fields, add support for graphql

* cmd/evm/testdata, core: use public 1559 gas names on API surfaces
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.

3 participants