SimulatedBackendClient: Fix validation error for eth_getBlockByNumber type checks - #7763
Conversation
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
… into bug-56284-simulated_backend_fix
prashantkumar1982
changed the title
… into bug-56284-simulated_backend_fix
| if isFullTx { | ||
| return errors.New("SimulatedBackendClient doesn't support full transactions for eth_getBlockByNumber") | ||
| } |
There was a problem hiding this comment.
I thought this would be OK to leave this in, since using *evmtypes.Block with isFullTx==false is a valid thing to do normally. Was it actively causing a problem?
There was a problem hiding this comment.
BlockHistoryEstimator always calls this with isFullTx = true.
See here:
chainlink/core/chains/evm/gas/block_history_estimator.go
Lines 591 to 593 in a51bafd
So if we allow this to fail on isFullTx==true, then BlockHistoryEstimator can't use the SimulatedBackendClient.
There was a problem hiding this comment.
I was thinking that returning an error for that unsupported case could be a feature, since without an error the dev could be misled to believe there are just empty blocks without txs 🤷 I don't know the surrounding context well though
There was a problem hiding this comment.
I understand, but this is a test utility.
If a dev uses that feature for a test, but then makes incorrect assumptions, then they are just writing an invalid test.
Assuming that they got empty blocks without Tx is their fault, as this is a test class, and they should be setting this to respond with the right Txs that they expect.
There was a problem hiding this comment.
But why set them up for failure in the first place? This code does not support that call. Why is it better to return the wrong answer than to refuse to answer?
There was a problem hiding this comment.
I understand, but this is a test utility.
Also, I fundamentally disagree with having lower standards for test utilities. It is a burden on development and undermines our ability to actually confirm production quality with the tests.
There was a problem hiding this comment.
This is not setting them up for failure or a lower standard for test utility.
Previously, aka before *evmtypes.Block was introduced, we always had isFullTx = False in all our production use-cases. So whoever wrote this simulated client, explicitly returned an error if that was true.
Now BlockHistoryEstimator actually sets isFullTx = true for a genuine case. So we removed that check, as it is no longer a correct assumption.
You could surely add the check back with conditional logic, saying if it is *evmtypes.Head, and isFullTx=True, then return an error.
But that check is not guarding or setting a higher bar for tests.
A developer who uses this SimulatedClient for testing has to know that it is a test utility, and it will only behave as it is set up to behave. It will return exactly the Blocks that the tester configured it to return. That specific check on IsFullTx cannot help or protect anyone if they're writing an invalid test case.
There was a problem hiding this comment.
it will only behave as it is set up to behave.
I think there is some confusion here. This code as written will never return txes. It cannot be configured by the caller in the current state to do that. Which test exactly is using this utility and requesting full blocks? It would only be valid to do that if you happen to require all empty blocks for your test case.
2 participants
Context: https://app.shortcut.com/chainlinklabs/story/56284/simulatedbackendclient-is-making-incorrect-type-assumptions-for-eth-getblockbynumber?stories_sort_by=id&stories_group_by=epic_id
#1 I had to refactor, and move Block, Transaction and TxType structures from evm.gas package to evm.types package, to avoid a circular import dependency.
#2 eth_getBlockByNumber call verification should allow the result type to be Head or Block. Both are valid now.