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.

Sign up for GitHub

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

Jump to bottom

EIP1559 #4155

Merged
merged 42 commits into from
Jul 21, 2021
Merged

EIP1559 #4155

merged 42 commits into from
Jul 21, 2021

Conversation

spacesailor24
Copy link
Contributor

@spacesailor24 spacesailor24 commented Jun 25, 2021 •
edited

Uses ethers.js' implementation/defaults reasoning as inspiration

closes #4150

NOTE Extra file changes are a result of branch off #4118, since it contains the needed EIP-1559 changes

@spacesailor24 spacesailor24 added the 1.x 1.0 related issues label Jun 25, 2021
@render
Copy link

render bot commented Jun 25, 2021

Your Render PR Server URL is https://web3-js-pr-4155.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c3akij50mal6e6ns4n1g.

ricmoo
ricmoo reviewed Jun 27, 2021
View reviewed changes
Copy link
Contributor

@ricmoo ricmoo left a comment

Choose a reason for hiding this comment

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

Keep in mind that EIP-1559 transactions also have an accessList, so this section might need some re-ordering.

There are also lots of decisions to make about incompatible properties (e.g. maxFeePerGas and gasPrice specifies).

GregTheGreek
GregTheGreek requested changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@GregTheGreek GregTheGreek left a comment

Choose a reason for hiding this comment

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

First glance, need to take a more thorough look

packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
@spacesailor24 @GregTheGreek
Fix type in error catch
f963659 
Co-authored-by: Gregory Markou <16929357+GregTheGreek@users.noreply.github.com>
@spacesailor24
Copy link
Contributor Author

Thank you @kchia , @ricmoo , and @GregTheGreek for taking a look at this

I've

  • added a _handleTxType method that includes property checking as @ricmoo mentioned (copies checks from Ethers)
  • updated maxFeePerGas calculation as per Ethers
  • updated _handleTxPricing to use gasPrice for maxPriorityFeePerGas and maxFeePerGas if network supports EIP-1559, but user provided gasPrice as per Ethers

Thank you @ricmoo for your R&D :)

@coveralls
Copy link

coveralls commented Jun 30, 2021 •
edited

Pull Request Test Coverage Report for Build 1050921428

  • 4 of 45 (8.89%) changed or added relevant lines in 2 files are covered.
  • 281 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-0.8%) to 75.42%
Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/web3-core-helpers/src/formatters.js 3 4 75.0%
packages/web3-eth-accounts/src/index.js 1 41 2.44%
Files with Coverage Reduction New Missed Lines %
packages/web3-core-requestmanager/src/jsonrpc.js 1 70.0%
packages/web3-core-helpers/src/formatters.js 7 83.83%
packages/web3-core-helpers/lib/formatters.js 21 80.72%
packages/web3-utils/src/utils.js 27 9.92%
packages/web3-core-helpers/src/errors.js 29 1.56%
packages/web3-utils/src/soliditySha3.js 34 3.43%
packages/web3-eth-accounts/lib/index.js 36 85.89%
packages/web3-utils/src/index.js 43 31.38%
packages/web3-eth-accounts/src/index.js 83 26.12%
Totals Coverage Status
Change from base Build 1015496704: -0.8%
Covered Lines: 3176
Relevant Lines: 3986
💛 - Coveralls

@spacesailor24 spacesailor24 marked this pull request as ready for review June 30, 2021 08:42
@corymsmith
Copy link
Contributor

Any updates on this @spacesailor24, need any help?

Hey there! Thanks for the offer, looks like we just got the failing tests to pass. The team is going to have a sync call around this tomorrow as a sanity check, then this PR will most likely get merged and released as an RC tomorrow

Awesome, once its released I can test it out in our staging environment, thanks for the update!

@spacesailor24 @jdevcs
EIP 1559 Debug #2 (#4171)
b3ab709 
* Add additional EIP-2930 and EIP-1559 tests

* Update gasPrice to geth default minimum

* Replace hex number with number string

* Add test skip if using Ganache

* Add done function to ganache skip

* Add done function to ganache skip

* Update gasPrice params

* Bump gasPrice to Geth default

* Bump docker geth version to stable

* Gas price type number for 1 Gwei

* gasPrice type and backfills timeout change

* gasPrice type and test acct creation

* added nonce for geth auto nonce issue, removed auto acct creation

* Remove redundant London test. Add gasLimit field to London test

* Add validation for tx.maxPriorityFeePerGas and tx.maxFeePerGas

* Add additional 1559 tests

Co-authored-by: jdevcs <junaid@chainsafe.com>
Co-authored-by: jdevcs <junaid@chainsafe.io>
@spacesailor24 spacesailor24 dismissed stale reviews from ghost via b3ab709 July 19, 2021 22:22
GregTheGreek
GregTheGreek requested changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@GregTheGreek GregTheGreek left a comment

Choose a reason for hiding this comment

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

Partial review.

packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
GregTheGreek
GregTheGreek requested changes Jul 20, 2021
View reviewed changes
packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
packages/web3-eth-accounts/src/index.js Outdated Show resolved Hide resolved
spacesailor24 and others added 6 commits July 20, 2021 09:44
@spacesailor24 @GregTheGreek
Update error message
e7d3833 
Co-authored-by: Gregory Markou <16929357+GregTheGreek@users.noreply.github.com>
@spacesailor24 @GregTheGreek
Update error message
d9c509a 
Co-authored-by: Gregory Markou <16929357+GregTheGreek@users.noreply.github.com>
@spacesailor24
Merge branch 'wyatt/eip1559' of github.com:ChainSafe/web3.js into wya…
2a90980 
…tt/eip1559
@spacesailor24
Update use of tx.type
7324eec
@spacesailor24
Replace hardfork strings with enum
d4f1363
@spacesailor24
Type check refactors for _handleTxPricing
f2ca0d8
@spacesailor24 spacesailor24 dismissed a stale review via f2ca0d8 July 21, 2021 00:49
@spacesailor24 spacesailor24 mentioned this pull request Jul 21, 2021
Closed
@spacesailor24 spacesailor24 merged commit 6ba267b into 1.x Jul 21, 2021
@spacesailor24 spacesailor24 deleted the wyatt/eip1559 branch July 21, 2021 19:31
@spacesailor24 spacesailor24 mentioned this pull request Jul 21, 2021
Merged
@spacesailor24
Copy link
Contributor Author

@corymsmith v1.5.0-rc.0 has been released

@ricmoo
Copy link
Contributor

ricmoo commented Jul 21, 2021

FYI, if you need more test cases, here are the ethers generated (with EthereumJS) testcases: https://github.com/ethers-io/ethers.js/blob/master/packages/testcases/testcases/typed-transactions.json.gz

@luu-alex luu-alex linked an issue Jul 22, 2021 that may be closed by this pull request
Implement EIP-1559 #4105
Closed
This was referenced Feb 1, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreMiras AndreMiras AndreMiras left review comments

@ricmoo ricmoo ricmoo left review comments

@kchia kchia kchia left review comments

@GregTheGreek GregTheGreek GregTheGreek approved these changes

@luu-alex luu-alex luu-alex approved these changes

@jdevcs jdevcs Awaiting requested review from jdevcs jdevcs is a code owner

Assignees
No one assigned
Labels
1.x 1.0 related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revert Geth Docker version change after EIP1559 support is added Implement EIP-1559
10 participants
@spacesailor24 @coveralls @liamaharon @corymsmith @ricmoo @AndreMiras @kchia @GregTheGreek @luu-alex @jdevcs