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

feat: adjust eip-1559 #3331

Closed
wants to merge 1 commit into from
Closed

feat: adjust eip-1559 #3331

wants to merge 1 commit into from

Conversation

rkdud007
Copy link

@rkdud007 rkdud007 commented Nov 8, 2022

Add option to adjust EIP-1559 gas fields in the Hardhat Network config

I opened an issue few days ago. I changed some values and add one unit-test function.
issue number is #3298
hope hardhat team can take a look at my code and give me feedbacks!

  • Because this PR includes a bug fix, relevant tests have been included.
  • Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
  • I didn't do anything of this.

Add option to adjust EIP-1559 gas fields in the Hardhat Network config
@changeset-bot
Copy link

changeset-bot bot commented Nov 8, 2022

⚠️ No Changeset found

Latest commit: 2b3a342

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Nov 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hardhat ✅ Ready (Inspect) Visit Preview Nov 8, 2022 at 0:58AM (UTC)
hardhat-storybook ✅ Ready (Inspect) Visit Preview Nov 8, 2022 at 0:58AM (UTC)

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

This issue is also being tracked on Linear.

We use Linear to manage our development process, but we keep the conversations on Github.

LINEAR-ID: fbfa417b-1b6d-488c-a456-80d3140012a9

@fvictorio
Copy link
Member

Hi @rkdud007, sorry for not reviewing your PR before.

There are a couple of problems here. First, it has compilation errors. Second, I don't think the behavior is correct, and there are some edge cases that are not handled (what happens if you set both gasPrice and maxFeePerGas? I think that should be an error; in fact, I think we should only add the max priority fee option, and that gasPrice works as max fee per gas when the priority is set).

Besides, there are no tests and the linter fails.

I think the first step here is to properly define the feature and the different behaviors. I opened #3418 to track that effort.

In the meantime, I'm going to close this PR for bookkeeping reasons. After we properly define the feature, feel free to open a new PR. But please make sure that 1) it compiles, 2) the linter passes and 3) it has proper tests.

@fvictorio fvictorio closed this Dec 12, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023
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.

2 participants