-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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/gasprice: implement feeHistory API #23033
Conversation
8969be1
to
1f6ca7b
Compare
return 0, nil, nil, nil, errInvalidPercentiles | ||
} | ||
} | ||
var ( |
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 can simplify the thing like this
func (f *Oracle) resolveLastBlockNumber(last rpc.BlockNumber) (*types.Block, uint64, bool, error) {
var (
pending *types.Block
noHead bool
)
if last == rpc.PendingBlockNumber {
pending, _ = f.backend.BlockByNumber(ctx, last)
if pending != nil {
return pending, pending.NumberU64(), false, nil
}
last, noHead = rpc.LatestBlockNumber, true
}
if last == rpc.LatestBlockNumber {
latestHeader, err := f.backend.HeaderByNumber(ctx, rpc.LatestBlockNumber)
if err == nil {
last = rpc.BlockNumber(latestHeader.Number.Uint64())
return nil, last, noHead, nil
}
return nil, 0, false, err
}
return nil, last, false, nil
}
The headBlockNumber
actually is unnecessary. If pending is not nil and it's requested for processing later, we can use the pending
field here for checking, instead of using headBlockNumber
And also the latestHeader
is also kind of unnecessary since retrieve the header again is cheap because of the cache.
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.
And we can call this function like this. It's much cleaner.
pending, last, noHead, err := f.resolveLastBlockNumber(lastBlockNumber)
if err != nil {
return 0, nil, nil, nil, err
}
if noHead {
blockCount--
if blockCount == 0 {
return
}
}
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.
Actually I do need headBlockNumber
to enforce maxHistory
and avoid requesting future blocks. But factoring out the block number resolving logic is a great idea, I think it's a lot more readable now. Also you're right about latestHeader
, saving it is not really imporant and not doing so makes the code simpler.
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.
The headBlockNumber
is last
returned by resolveLastBlockNumber
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.
That's not true if lastBlockNumber
was an explicit block number different from the head. We still need the head though to apply maxHistory
restriction and prevent future block requests. So I think it's better to also integrate these checks into resolveBlockRange
that can also limit blockCount
if necessary.
} | ||
if lastBlockNumber == rpc.LatestBlockNumber { | ||
lastBlockNumber = headBlockNumber | ||
} else if pendingBlock == nil && lastBlockNumber > headBlockNumber { |
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.
Shouldn't we reject this request in the first place? I don't think request the transaction fees of the future blocks make any sense.
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.
Right, it's probably better to reject it explicitly. Now I return an error in this case.
internal/ethapi/api.go
Outdated
type feeHistoryResults struct { | ||
FirstBlock rpc.BlockNumber | ||
Reward [][]*big.Int //hexutil.Big | ||
BaseFee []*big.Int //hexutil.Big |
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.
Why not use the hexutil.Big
?
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 wanted to but then forgot about it. Did it now.
) | ||
if pendingBlock, pendingReceipts, lastBlockNumber, blockCount, err = oracle.resolveBlockRange(ctx, lastBlockNumber, blockCount, maxHistory); err != nil { | ||
return | ||
} |
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 would prefer to return directly if the block count is 0 here. It's cleaner
This PR implements the
feeHistory
EIP-1559 gas price API specified here:ethereum/execution-specs#212
Original proposal:
https://gist.github.com/zsfelfoldi/473e29106d38525de6b4413e2ebcddf1