-
Notifications
You must be signed in to change notification settings - Fork 249
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
add feeHistory spec #212
add feeHistory spec #212
Conversation
json-rpc/spec.json
Outdated
"params": [ | ||
{ | ||
"name": "blockCount", | ||
"description": "Number of blocks in the requested range.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider mentioning what a value of 0
does/means. Also, are there any constraints on this (limit to number one can request)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
json-rpc/spec.json
Outdated
"name": "lastBlock", | ||
"description": "Last block number of the requested range.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming this to "newest" instead of "last". Depending on your frame of reference, "last" may be closer to genesis than head, or it may be closer to head than genesis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
json-rpc/spec.json
Outdated
{ | ||
"$ref": "#/components/schemas/BlockNumber" | ||
}, | ||
{ | ||
"$ref": "#/components/schemas/BlockNumberTag" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create a new type for BlockNumberOrTag
since it comes up a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
json-rpc/spec.json
Outdated
"title": "firstBlock", | ||
"description": "First block number of the returned range.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider "newest" here if you decide to change the above to "newest" so naming is consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean oldest I guess :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, all the more reason to change this wording! I thought by "first" you meant "latest" (most recent, first block counting down from now, etc.). 😁
json-rpc/spec.json
Outdated
}, | ||
"baseFee": { | ||
"title": "baseFeeArray", | ||
"description": "An array of block base fees (including the next block after the returned range).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems odd to me that this would return more blocks than requested. If the user wants +1, they can just ask for +1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This info is derived from the requested blocks (the next base fee always follows from the last block). It makes sense to specify it like this because the pending base fee is basically the most important and it has nothing to do with the pending state that is not supported at all by some backends (so it either couldn't be queried or would be an ugly corner case with only the baseFee returned for the pending block). On the other hand if pending state is supported and the pending block is queried then we can even predict even the after-pending block's base fee because it follows from the pending state and it is a good indicator of congestion in the mempool. There isn't even a way to specify the after-pending block as lastBlock. So I'd strongly like to keep it this way and specify that the function returns info derivable from the requested blocks (hence the extra baseFee value).
json-rpc/spec.json
Outdated
}, | ||
"gasUsedRatio": { | ||
"title": "gasUsedArray", | ||
"description": "An array of block gas used ratios.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a decimal number between 0 and 1? Might be good to provide a bit more description here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@MicahZoltu @lightclient does this seem ready to merge to you? |
json-rpc/spec.json
Outdated
}, | ||
"baseFee": { | ||
"title": "baseFeeArray", | ||
"description": "An array of block base fees (including the next block after the newest of the returned range because this value can be derived from the newest block).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this null if the block has no base fee?
I'm curious about this. What will be the criteria to accept modifications to this spec? Will accepting a new method imply that all the clients and developer networks (e.g. ganache, hardhat, remix) need to implement it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this PR! In general, it would be good to avoid discussing things in parentheses. It would be better to state things explicitly. I'd also like the actual type information to be encoded in the schema.
json-rpc/spec.json
Outdated
{ | ||
"name": "eth_feeHistory", | ||
"summary": "Transaction fee history", | ||
"description": "Returns transaction base fee and effective priority fee history for the requested/supported block range.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the behaviour be before EIP-1559 blocks? I think it would be good to mention that upfront in the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, these should be listed "base fee per gas" and "effective priority fee per gas".
json-rpc/spec.json
Outdated
"params": [ | ||
{ | ||
"name": "blockCount", | ||
"description": "Number of blocks in the requested range (positive integer; less may be returned if requesting the entire specified range is not supported).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "Number of blocks in the requested range (positive integer; less may be returned if requesting the entire specified range is not supported).", | |
"description": "Number of blocks in the requested range (less may be returned if requesting the entire specified range is not supported).", |
The schema should denote the type.
It's also not clear to me what "not supported" means?
json-rpc/spec.json
Outdated
}, | ||
{ | ||
"name": "newestBlock", | ||
"description": "Newest (highest number) block of the requested range (the range between headBlock-4 and headBlock are guaranteed to be supported while the pending block and older history are optional to support).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "Newest (highest number) block of the requested range (the range between headBlock-4 and headBlock are guaranteed to be supported while the pending block and older history are optional to support).", | |
"description": "Highest number block of the requested range. The range between headBlock-4 and headBlock are guaranteed to be supported while the pending block and older history are optional to support.", |
json-rpc/spec.json
Outdated
}, | ||
{ | ||
"name": "rewardPercentiles", | ||
"description": "A list of percentile values (0..100 floating point) to sample from each block's miner rewards in ascending order, weighted by gas used.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "A list of percentile values (0..100 floating point) to sample from each block's miner rewards in ascending order, weighted by gas used.", | |
"description": "A list of percentile values to sample from each block's miner rewards in ascending order, weighted by gas used.", |
Please use the schema
field to communicate the type information.
json-rpc/spec.json
Outdated
"properties": { | ||
"oldestBlock": { | ||
"title": "oldestBlock", | ||
"description": "Oldest (lowest number) block of the returned range.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "Oldest (lowest number) block of the returned range.", | |
"description": "Lowest number block of the returned range.", |
json-rpc/spec.json
Outdated
}, | ||
"baseFee": { | ||
"title": "baseFeeArray", | ||
"description": "An array of block base fees (including the next block after the newest of the returned range because this value can be derived from the newest block).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "An array of block base fees (including the next block after the newest of the returned range because this value can be derived from the newest block).", | |
"description": "An array of block base fees per gas. This includes the next block after the newest of the returned range, because this value can be derived from the newest block.", |
json-rpc/spec.json
Outdated
}, | ||
"gasUsedRatio": { | ||
"title": "gasUsedArray", | ||
"description": "An array of block gas used ratios (gasUsed/gasLimit; 0..1 floating point).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "An array of block gas used ratios (gasUsed/gasLimit; 0..1 floating point).", | |
"description": "An array of block gas used ratios. This is calculated by dividing the gasUsed by the gasLimit.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the schema to denote the type.
json-rpc/spec.json
Outdated
"type": "array", | ||
"items": { | ||
"title": "gasUsedRatio", | ||
"description": "The ratio of gas used and gas limit (0..1 floating point).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "The ratio of gas used and gas limit (0..1 floating point).", | |
"description": "The ratio of gas used and gas limit.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the schema to denote the type.
json-rpc/spec.json
Outdated
}, | ||
"reward": { | ||
"title": "rewardArray", | ||
"description": "A two-dimensional array of block miner rewards (effective priority fees) at the requested block percentiles.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "A two-dimensional array of block miner rewards (effective priority fees) at the requested block percentiles.", | |
"description": "A two-dimensional array of effective priority fees per gas at the requested block percentiles.", |
@@ -1135,6 +1223,17 @@ | |||
"pending" | |||
] | |||
}, | |||
"BlockNumberOrTag": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you just pull this out to be more explicit?
This is a great question. I think the best way to go about it is have endpoints be discussed and proposed as PRs and then merge those changes into the spec repo as "pre-releases". Every month or so, we would need to have a synchronous call with the stakeholders (like the teams you've mentioned) and client devs to get a thumbs up on the release candidate, then we can release a new version of the spec. It may also be useful to target a date where all major tooling has adapted to support the latest |
I have a mild preference for having a branch that we merge PRs like this into that is named something like We could also have a branch for A workflow like this makes it very easy to find out what is the current specification (broadly speaking, implemented by one or more clients) as well as making it easy to see what is currently up for "official" proposal and easy to generate a diff or a PR for discussing current proposals. |
@alcuadrado good question! As @lightclient and @MicahZoltu have pointed out, this process is still new and we haven't figured out the kinks yet. I'm in favor of the general approach of having multiple branch and review periods. I'll try and draft up a proposal today/tomorrow. As for this specific PR, I'm inclined to merge it for a couple reasons:
Ideally, tooling/infrastructure should support this, but having full 1559 support on Day 1 of the London fork isn't a strict requirement as legacy txns will still be allowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I think just a couple missed per gas
es and then good to go.
json-rpc/spec.json
Outdated
{ | ||
"name": "eth_feeHistory", | ||
"summary": "Transaction fee history", | ||
"description": "Returns base fee and transaction effective priority fee history for the requested block range if available. The range between headBlock-4 and headBlock is guaranteed to be available while retrieving data from the pending block and older history are optional to support. For pre-EIP-1559 blocks the gas prices are returned as rewards and zeroes are returned for the base fee.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "Returns base fee and transaction effective priority fee history for the requested block range if available. The range between headBlock-4 and headBlock is guaranteed to be available while retrieving data from the pending block and older history are optional to support. For pre-EIP-1559 blocks the gas prices are returned as rewards and zeroes are returned for the base fee.", | |
"description": "Returns base fee per as and transaction effective priority fee per gas history for the requested block range if available. The range between headBlock-4 and headBlock is guaranteed to be available while retrieving data from the pending block and older history are optional to support. For pre-EIP-1559 blocks the gas prices are returned as rewards and zeroes are returned for the base fee per gas.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
"description": "Number of blocks in the requested range. Between 1 and 1024 blocks can be requested in a single query. Less than requested may be returned if not all blocks are available.", | ||
"required": true, | ||
"schema": { | ||
"$ref": "#/components/schemas/Integer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an integer between 1 and 1024. IMO anything we can encode in the schema, we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, I suppose the description is good enough for now. Eventually we'll should encode in the schema though.
I added the missing "per gas" where requested and squashed my commits. I think it's mergeable now unless there is a further issue :) |
@zsfelfoldi is the gist of the original proposal correct? Shouldn't be this one instead? |
+1. The HackMD link seems wrong. |
This PR adds the feeHistory API function spec.
See also: