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 #2432

Closed
wants to merge 39 commits into from
Closed

eth_feeHistory #2432

wants to merge 39 commits into from

Conversation

RatanRSur
Copy link
Contributor

@RatanRSur RatanRSur commented Jun 14, 2021

The spec for this implementation is listed in the issue below.

Fixed Issue(s)

fixes #2430

Changelog

RatanRSur added 17 commits June 16, 2021 21:30
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@RatanRSur RatanRSur marked this pull request as ready for review June 17, 2021 03:37
@RatanRSur RatanRSur changed the title fee history eth_feeHistory Jun 17, 2021
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

suggestions re: arithmetic and input validation

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

LGTM, just need to address effectivePriorityFee for legacy transactions post-london

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

🚢 (after unit tests)

@RatanRSur RatanRSur enabled auto-merge (squash) June 21, 2021 20:24
@@ -619,7 +620,7 @@ public KeyPair generateKeyPair() {
private boolean hasTransactions = true;
private TransactionType[] transactionTypes = TransactionType.values();
private Optional<Address> coinbase = Optional.empty();
private Optional<Long> baseFee = Optional.empty();
private Optional<Optional<Long>> maybeBaseFee = Optional.empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional<Optional<Long>> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah because we need a way to encode if the basefee is unspecified. This is important for knowing whether to use a default base fee or the optional.empty the caller gives explicitly to the builder.

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@matkt
Copy link
Contributor

matkt commented Jun 22, 2021

I'm just concerned about the performance of this API when blockcount == 500 on calaveras. I wanted to try and after 2 minutes and several thread blocked we have the result. I also wanted to test with a blockcount == 10 starting from the block 6400 and I had 23 seconds. These blocks are filled by transactions

What is the blockcount requested in the common case ?

@RatanRSur
Copy link
Contributor Author

I think block counts of 500 would be very unusual but 10 sounds reasonable.

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>
@garyschulte
Copy link
Contributor

I think block counts of 500 would be very unusual but 10 sounds reasonable.

How about we silently map anything >10 to 10. We might fail some test cases in hive, but this seems like an acceptable tradeoff for now.

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

🚢

@RatanRSur RatanRSur closed this Jun 22, 2021
auto-merge was automatically disabled June 22, 2021 18:13

Pull request was closed

@garyschulte garyschulte added the documentation Improvements or additions to documentation label Jun 24, 2021
@rolandtyler rolandtyler removed the documentation Improvements or additions to documentation label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add feeHistory JSON RPC API
4 participants