8000 A list of APIs to be changed or added for dynamic gas price · Pull Request #1452 · klaytn/klaytn · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Aug 19, 2024. It is now read-only.

A list of APIs to be changed or added for dynamic gas price #1452

Merged
21 commits merged into from Jun 30, 2022
Merged

A list of APIs to be changed or added for dynamic gas price #1452

21 commits merged into from Jun 30, 2022

Conversation

ghost
Copy link
@ghost ghost commented Jun 22, 2022

Proposed changes

  • A list of APIs to be changed or added while applying KIP-71 Dynamic gas fee pricing mechanism.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

@ghost ghost changed the title Api dynamic gas price A list of APIs to be changed or added for dynamic gas price Jun 22, 2022
@ghost ghost self-assigned this Jun 22, 2022
@ghost ghost requested a review from ethan-kr as a code owner June 22, 2022 22:44
@ghost ghost requested a review from jiseongnoh as a code owner June 23, 2022 06:14
@@ -62,22 +63,33 @@ var (
errInvalidUpperBound = errors.New("upperboundbasefee cannot be set lower than lowerboundbasefee")
)

// TODO-Klaytn-Governance: Refine this API and consider the gas price of txpool
// GasPriceAt returns the baseFeePerGas of the given block in peb.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// GasPriceAt returns the baseFeePerGas of the given block in peb.
// GasPriceAt returns the baseFeePerGas of the given block in peb.
// It returns unit price by using governance if there is no base fee set in header.

@@ -308,7 +311,8 @@ func (api *EthereumAPI) GasPrice(ctx context.Context) (*hexutil.Big, error) {

// MaxPriorityFeePerGas returns a suggestion for a gas tip cap for dynamic fee transactions.
func (api *EthereumAPI) MaxPriorityFeePerGas(ctx context.Context) (*hexutil.Big, error) {
return api.publicKlayAPI.MaxPriorityFeePerGas(ctx)
// TODO-Klaytn maybe need to branch kip71 hardfork

Choose a reason for hiding this comment

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

We'd better make sure this. Do we need to do different action based on the hardfork or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this TODO comment.
api.GasPrice will return unit price before hard fork and return next block'sm baseFee after hard fork.
@bobpkr Please let me know if i misunderstood

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to discuss this! What to do with maxPriorityFeePerGas...! Please share your opinions.

Copy link
Author

Choose a reason for hiding this comment

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

First of all, you are right.
I heard that @kjeom will be more organized and will give you some feedback.

bobpkr added 2 commits June 29, 2022 12:52
- add a condition for checking maxFeePerGas and maxPriorityFeePerGas
@ghost ghost merged commit 69c1126 into klaytn:dynamicGasPrice Jun 30, 2022
@ghost ghost deleted the api-dynamicGasPrice branch June 30, 2022 02:13
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0