-
Notifications
You must be signed in to change notification settings - Fork 20.3k
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
graphql: fee history fields #24452
graphql: fee history fields #24452
Conversation
I applied the review comments and fixed some minor issues. Ready for review |
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.
Left a few comments.
if err != nil || tx == nil { | ||
return nil, err | ||
} | ||
header, err := t.block.resolveHeader(ctx) |
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.
for the non-finalized transaction, the t.block
can be nil. Please add one more check 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.
Ah right. I had copied some stuff from EffectiveGasPrice
which has the same issue. Added a check in both methods.
Do you think it makes sense to compute and use the latest block's nextBaseFee for pending txes?
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.
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.
I think it's safer to return an error rather than 'speculate' on the tip, for pending transactions.
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 need to decide what's the semantic of EffectiveGasPrice
, EffectiveTip
and NextBaseFeePerGas
in case the basefee is not existent.
Currently it's returned as 0 in this PR, it's good to me though. Otherwise code is good to me.
in which cases is it not existant? Do you mean blocks prior to the fork? |
Yes that's what Gary is referring to. Right now these fields ( |
This pr, as is, what happens if you compare json-rpc versus graphql on blocks prior to the fork? |
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.
LGTM
This PR adds the `NextBaseFeePerGas` to `Block` and `EffectiveTip` to `Transaction` to make it easier for clients to compute fee history themselves via graphql queries.
This PR adds the `NextBaseFeePerGas` to `Block` and `EffectiveTip` to `Transaction` to make it easier for clients to compute fee history themselves via graphql queries.
This PR adds the `NextBaseFeePerGas` to `Block` and `EffectiveTip` to `Transaction` to make it easier for clients to compute fee history themselves via graphql queries.
This PR adds the
NextBaseFeePerGas
toBlock
andEffectiveTip
toTransaction
to make it easier for clients to compute fee history themselves. Note I made the tx reward sorting logic in our fee history algorithm stable so clients can compute consistent results.Example client script: https://gist.github.com/s1na/851ad57e8fca7c74b2464207b2e99021
Result of a run on a few Goerli blocks:
Fixes #24386