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

eth_feeHistory (EIP 1559) #4191

Merged
merged 11 commits into from
Jul 24, 2021
Merged

eth_feeHistory (EIP 1559) #4191

merged 11 commits into from
Jul 24, 2021

Conversation

spacesailor24
Copy link
Contributor

@spacesailor24 spacesailor24 commented Jul 23, 2021

Add missing getFeeHistory call that's made available via EIP 1559

NOTE

  • RPC docs say that blockCount should be a hex string, but in my testing using Infura URLs on Ropsten and Rinkeby, only integers seem to be supported. Additionally, for newestBlock, block tags (e.g. earliest, latest, etc) don't seem to be supported, only hex strings To combat this, I added input formatters to blockCount and lastBlock
  • Should be merged alongside eth-feehistory docs #4190

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

render bot commented Jul 23, 2021

@coveralls
Copy link

coveralls commented Jul 23, 2021

Pull Request Test Coverage Report for Build 1061334675

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • 255 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+2.5%) to 75.322%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/web3-utils/src/utils.js 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
packages/web3-core-requestmanager/src/jsonrpc.js 1 70.0%
packages/web3-core-helpers/src/formatters.js 8 83.83%
packages/web3-core-helpers/src/errors.js 29 1.56%
packages/web3-utils/src/utils.js 30 8.46%
packages/web3-utils/src/soliditySha3.js 34 3.43%
packages/web3-utils/src/index.js 43 31.38%
packages/web3-eth-accounts/src/index.js 110 26.12%
Totals Coverage Status
Change from base Build 1056532201: 2.5%
Covered Lines: 3178
Relevant Lines: 3992

💛 - Coveralls

packages/web3-eth/src/index.js Show resolved Hide resolved
callback?: (error: Error, gasPrice: string) => void
): Promise<string>;

getFeeHistory(
callback?: (error: Error, feeHistory: FeeHistoryResult) => void
Copy link
Contributor

Choose a reason for hiding this comment

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

This should contain blockCount, newestBlock and rewardPercentiles?

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 you mixed up getGasPrice and getFeeHistory?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also rewardPercentiles should be marked as optional

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think the docs here are misleading https://github.com/ChainSafe/web3.js/pull/4190/files#diff-b1b5b8a010cd61ad800e528887f49730db04064d59d3ee5ed14de0f49f2ba11cR740

Looks like you still have to pass an empty array so I'm not sure if web3 should just do that as the default if that param isn't set? Would be more user friendly than having to pass an empty array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GregTheGreek Whoops, forgot to push, thank you

@corymsmith I think web3 defaulting an empty array would be nice for the user, but we can't have two optional arguments in typescript, so I'm not sure how we could do this

Copy link
Contributor

Choose a reason for hiding this comment

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

True, I suppose the only way to do it would be to add a formatter that formats null into []

@corymsmith
Copy link
Contributor

Add missing getFeeHistory call that's made available via EIP 1559

NOTE

  • RPC docs say that blockCount should be a hex string, but in my testing using Infura URLs on Ropsten and Rinkeby, only integers seem to be supported. Additionally, for newestBlock, block tags (e.g. earliest, latest, etc) don't seem to be supported, only hex strings
  • Should be merged alongside eth-feehistory docs #4190

From looking here that looks correct, https://github.com/ethereum/go-ethereum/blob/9624f92edef5e0a76a97efd302e983077acb6e35/eth/gasprice/feehistory.go#L200

blocks which you named as blockCount is an int and lastBlock which you named as newestBlock is a UInt64 but is formatted that way using the inputBlockNumberFormatter

@@ -439,3 +446,10 @@ export interface StorageProof {
value: string;
proof: string[];
}

export interface FeeHistoryResult {
oldestBlock: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

This specifies oldestBlock is returned as a string but the tests are showing its returned as a number here:
https://github.com/ChainSafe/web3.js/pull/4191/files#diff-99b1655ae7b89f474d7f663a07e45ccaa75c9270bb15bf852f1f0bdf2164f711R23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I just blindly followed the RPC docs, thank you

@spacesailor24
Copy link
Contributor Author

Add missing getFeeHistory call that's made available via EIP 1559
NOTE

  • RPC docs say that blockCount should be a hex string, but in my testing using Infura URLs on Ropsten and Rinkeby, only integers seem to be supported. Additionally, for newestBlock, block tags (e.g. earliest, latest, etc) don't seem to be supported, only hex strings
  • Should be merged alongside eth-feehistory docs #4190

From looking here that looks correct, https://github.com/ethereum/go-ethereum/blob/9624f92edef5e0a76a97efd302e983077acb6e35/eth/gasprice/feehistory.go#L200

blocks which you named as blockCount is an int and lastBlock which you named as newestBlock is a UInt64 but is formatted that way using the inputBlockNumberFormatter

So it appears that the docs are wrong ☹️, I'll try to add some formatters to blockCount and newestBlock (which I want to rename to lastBlock) to make it easier for the user

spacesailor24 and others added 7 commits July 23, 2021 13:32
@spacesailor24 spacesailor24 marked this pull request as ready for review July 23, 2021 23:53
@spacesailor24 spacesailor24 merged commit 436e77a into 1.x Jul 24, 2021
@spacesailor24 spacesailor24 deleted the wyatt/feehistory branch July 24, 2021 00:33
@spacesailor24 spacesailor24 mentioned this pull request Jul 24, 2021
spacesailor24 added a commit that referenced this pull request Aug 26, 2021
* WIP

* Add missing fields for test runner

* Correct function arguments for getFeeHistory

* getFeeHistory tests with correct arguments

* Init utils.toNumber function

* Rename toNumber test to hexToNumber

* Add inputFormatters to getFeeHistory

* Rename newestBlock to lastBlock. Update types for blockCount and lastBlock

* Add additional tests with different input types

* Add missing function export

* eth-feehistory docs (#4190)

* updating docs

* updating example

* updating types and adding example of list

* Update docs/web3-eth.rst

* Update docs/web3-eth.rst

* Update docs/web3-eth.rst

Co-authored-by: alex <[email protected]>
Co-authored-by: Wyatt Barnes <[email protected]>

Co-authored-by: Alex <[email protected]>
Co-authored-by: alex <[email protected]>
spacesailor24 added a commit that referenced this pull request Aug 26, 2021
* Merge conflitcs

* Add unsubscribeByID (#4061)

* adding function unsubscribe by id

* adding an unsubscribe testcase

* adding testcase

* seperated unsubscribebyid to its own method

* adding testcases

* adding await

* fixing testcases

Co-authored-by: Alex <[email protected]>
Co-authored-by: Alex <[email protected]>

* ignore .md and docs (#4077)

* Merge conflicts

* Merge conflicts

* Release v1.4.0 (#4118)

* npm run build for 1.4.0-rc.0

* v1.4.0-rc.0

* 1.4.0 Geth version downgrade (#4149)

* Update pull Geth docker version from stable to pre-london (1.10.3)

* Update CHANGELOG

* v1.4.0

* remove underscore (#4069)

* removed some of the underscore methods in web3-core-method

* removed underscore from bzz

* adding subscriptions

* fixing up test cases

* changing variable names

* removed underscore from formatters.js

* removed underscore from request manager and abi

* removing underscore in the rest of the web3 packages

* fixing exports

* fixing failing testcases

* removing underscore from tests

* addressing feedback

* removing unwanted code from transaction

* removing underscore from remaining packages

* updating change log

* addressing feedback

* adding strict equality

* efficient short circuiting

* fixing test case

* Merge conflicts

* Update docs 2 (#4188)

* add nonce to send options.

* add nonce to send options.

* Update CHANGELOG.md

* linting

* updating docs

* update web3-shh.rst : whisper-overview broken link (#4170)

whisper overview has been moved from https://github.com/ethereum/go-ethereum/wiki/Whisper to https://eth.wiki/concepts/whisper/whisper-overview

* [Docs] Fixed a broken link (#4151)

Original link gives out a 404, replaced by the closest thing I could find.

* Possible typo in docs (#4088)

I think the author missed a "this" in the phrase

* Typically you will have only one Web3 connection, use const (#3967)

Co-authored-by: Gregory Markou <[email protected]>

* Removing deprecation notice for HttpProvider (#4008)

* Removing deprecation notice for HttpProvider

From my view, it is just a provider with less capabilities than websockets, but still widely useful — and widely used as well.

@frozeman @nivida @GregTheGreek any thoughts?

* Update include_package-core.rst

* Update CHANGELOG.md

Co-authored-by: Gregory Markou <[email protected]>
Co-authored-by: Alex <[email protected]>

* [Docs] Updated solidity example to modern syntax (#4147)

* [Docs] Updated solidity example to modern syntax

Replaced the old constructor function syntax with the modern one, added 'emit' to event calls, added pragma, added a valid bytes32 value and updated the JSON ABI.

* [Docs] Fixed a broken link (#1)

Original link gives out a 404, replaced by the proper guide.

Co-authored-by: Juan Alonso <[email protected]>

* Revert "[Docs] Fixed a broken link (#1)"

This reverts commit 0de1272.

Co-authored-by: Juan Alonso <[email protected]>

* added EIP-2718 and EIP-1559 documentation

* fixing typo

* addressing feedback

* changing possible types for maxPriorityFeePerGas

* updating maxFeePerGas type

* update changelog

Co-authored-by: exx8 <[email protected]>
Co-authored-by: starwalker00 <[email protected]>
Co-authored-by: mongolsteppe <[email protected]>
Co-authored-by: João Monteiro <[email protected]>
Co-authored-by: William Entriken <[email protected]>
Co-authored-by: Gregory Markou <[email protected]>
Co-authored-by: Ev <[email protected]>
Co-authored-by: Juan Alonso <[email protected]>

* eth_feeHistory (EIP 1559) (#4191)

* WIP

* Add missing fields for test runner

* Correct function arguments for getFeeHistory

* getFeeHistory tests with correct arguments

* Init utils.toNumber function

* Rename toNumber test to hexToNumber

* Add inputFormatters to getFeeHistory

* Rename newestBlock to lastBlock. Update types for blockCount and lastBlock

* Add additional tests with different input types

* Add missing function export

* eth-feehistory docs (#4190)

* updating docs

* updating example

* updating types and adding example of list

* Update docs/web3-eth.rst

* Update docs/web3-eth.rst

* Update docs/web3-eth.rst

Co-authored-by: alex <[email protected]>
Co-authored-by: Wyatt Barnes <[email protected]>

Co-authored-by: Alex <[email protected]>
Co-authored-by: alex <[email protected]>

* Update CHANGELOG (#4193)

Test don't run when only changes to `CHANGELOG.md` have been made

* Merge conflicts

* Merge conflicts

* Merge conflicts

* Merge conflicts

* Merge conflicts

* Merge conflicts

* Merge conflicts

* Merge conflicts

Co-authored-by: Alex <[email protected]>
Co-authored-by: Alex <[email protected]>
Co-authored-by: Gregory Markou <[email protected]>
Co-authored-by: exx8 <[email protected]>
Co-authored-by: starwalker00 <[email protected]>
Co-authored-by: mongolsteppe <[email protected]>
Co-authored-by: João Monteiro <[email protected]>
Co-authored-by: William Entriken <[email protected]>
Co-authored-by: Ev <[email protected]>
Co-authored-by: Juan Alonso <[email protected]>
Co-authored-by: alex <[email protected]>
Co-authored-by: jdevcs <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants