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

Update: using provided gas options for eip-1559 tx #5012

Merged

Conversation

chendatony31
Copy link
Contributor

Description

We should use provided gas options for eip-1559 tx same as the legacy ones, and avoid useless RPC requests

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:cov and my test cases cover all the lines and branches of the added code.
  • I ran npm run build with success.
  • I have tested the built dist/web3.min.js in a browser.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.

@coveralls
Copy link

coveralls commented May 9, 2022

Pull Request Test Coverage Report for Build 2782780642

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • 28 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.04%) to 72.213%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/web3-eth-accounts/src/index.js 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
packages/web3-eth-accounts/src/index.js 1 23.35%
packages/web3-eth-accounts/lib/index.js 27 85.62%
Totals Coverage Status
Change from base Build 2775256859: -0.04%
Covered Lines: 3404
Relevant Lines: 4436

💛 - Coveralls

@jdevcs jdevcs added 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels May 16, 2022
Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

@chendatony31 The change looks good. Can you please add at least one test to cover this scenario?

@chendatony31
Copy link
Contributor Author

Rebased

Copy link
Contributor

@jdevcs jdevcs left a comment

Choose a reason for hiding this comment

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

@chendatony31 change log update is also required

@chendatony31
Copy link
Contributor Author

Done @jdevcs

@nikoulai nikoulai requested a review from jdevcs July 7, 2022 07:45
@jdevcs jdevcs added Future Release For future release. and removed Review Needed Maintainer(s) need to review labels Jul 7, 2022
@jdevcs
Copy link
Contributor

jdevcs commented Jul 15, 2022

@chendatony31 due to recent merge of another PR there are merge conflicts, could you fix those.

@chendatony31
Copy link
Contributor Author

@jdevcs Any plan to merge this PR? As it has been about 3 months

This was referenced Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Future Release For future release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants