Improve RPC API compatibility for Ethers.js #1957
Conversation
Test tries to get the block
Instead of running a script we now use the mocha testing framework which provides much better output and makes it much easier to add testcases.
Update CI to use new docker image, since we now need to run node and npm as part of our tests.
The IPV6 addresses returned from the geth node were not working on CI so we convert them to IPV4 addresses.
Socket Security Report👍 No new dependency issues detected in pull request Socket.dev scan summary
Bot CommandsTo ignore an alert, reply with a comment starting with Powered by socket.dev |
|
Coverage from tests in coverage: 44.0% of statements across all listed packagescoverage: 47.7% of statements in consensus/istanbul coverage: 23.7% of statements in consensus/istanbul/announce coverage: 54.4% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 55.6% of statements in consensus/istanbul/core coverage: 45.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.4% of statements in consensus/istanbul/uptime coverage: 51.8% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/randomCommentID: 59959d96fe |
Add installed go binaries location to user's path
piersy
force-pushed
the
piersy/ethersjs-compatibility
branch
from
ccae81b
to
b354bc4
Compare
Codecov ReportBase: 54.82% // Head: 54.78% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1957 +/- ##
==========================================
- Coverage 54.82% 54.78% -0.04%
==========================================
Files 688 694 +6
Lines 92542 93242 +700
==========================================
+ Hits 50737 51084 +347
- Misses 37993 38316 +323
- Partials 3812 3842 +30
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
It will be inconsistent in both variants, right? Either it first returns 20M and then another value once the state is available, or it is first not returned and then suddenly is. How was the gas limit historically set? And how would people use it? |
This should mitigate the readme going out of date whenever we update go.
It's the other way around, the state is always available for the current block up to the 128th block, after that the state is pruned. So for our chain you'd expect to have state for a block for about But yes either way is inconsistent. Either:
The gas limit is set through a governance change modifying the blockchain parameters. I'm not really sure what people would do with the gas limit, maybe some people hardcode the gas limit on thier txs to the block gas limit because they don't want to call estimateGas but apart from that I can't think of anything. |
internal/ethapi/api.go
Outdated
| block["gasLimit"] = hexutil.Uint64(gasLimit) | ||
|
|
||
| // Providing nil as the currency address gets the gas price minimum for the native celo asset. | ||
| baseFee, err := b.GasPriceMinimumForHeader(ctx, nil, header) |
There was a problem hiding this comment.
Should we also mock the gasPriceMinimum for blocks that we don't have the state?
The same as we are doing with the GetBlockGasLimit where we are returning params.DefaultGasLimit which is set to 20M.
Both have different natures and the blockGasLimit will fluctuate only by governance (which is not that common), and the price is tied to usage. But we are already sending a possible "wrong" answer. Should we do the same with the baseFee? Or maybe we should put a nil in the gasLimit too if we don't have the state
There was a problem hiding this comment.
Same as the other comments, sorry for didn't check that before.
I agree that we should be consistent. Or we return null in both cases, or we return mocked things
There was a problem hiding this comment.
I'm leaning towards no mocking, because if we switch to actually putting these fields on the block then there will be a point when the fields don't exist. Otherwise we have to maintain this shim forever. But I'm really not sure.
ethersjs-api-check/test/test.ts
Outdated
| // Our blockchain client implementation returns a default gas limit when the | ||
| // actual gas limit cannot be retrieved. We cannot check fee data against a | ||
| // pruned block because getFeeData always requests the latest block. | ||
| it('block with pruned state still reports gas limit', async () => { |
There was a problem hiding this comment.
Trying to understand something.
This test will be triggered probably with state.
But you are reusing this test in the e2e one, to execute with state, prune it, and execute it again right? So that is the actual test that is checking the prunning, and this one executed by circle, will always return what we have in the state
There was a problem hiding this comment.
For the pruned one, you could also add a test checking that the baseFee is null using the same logic (if we keep the baseFee as it is, and not adding a default as I asked in another comment)
There was a problem hiding this comment.
@ponti, I don't understand why you think this test would be executed with state?
There was a problem hiding this comment.
Ok so just to clarify, the e2e test was controlling which test to run from the set of tests and would only run the tests for the the pruned state when it had pruned the state.
If the flag is set then the 'gasLimit' and 'baseFeePerGas' fields will not be returned on RPC blocks.
One for tests with state one for tests without. This makes executing each batch of tests easier because we can just grep for the describe headings.
Just to further signify that it is not for standalone use.
If the state is missing for gasLimit or baseFeePerGas or if there is some other failure when retrieving them then do not add them as fields to the block. Preivously in some cases a default value would have been added to the block and returned.
|
@gastonponti I've updated this PR to not return the fields on the block when we can't retrieve them. I've also split the tests up under 2 describe headings one for tests with state and one for tests without. And finally added a few more tests. Please take a look |
* Add minimal ts project * Add ethers.js dependency * Add etherejs api test (currently failing) Test tries to get the block * Add dummy data for gas limit field * Build ethersjs project before executing test * Return real gas limit in place of dummy data * Return baseFee if it is acessible * Use framework for ethers.js compatibility tests Instead of running a script we now use the mocha testing framework which provides much better output and makes it much easier to add testcases. * Store package lock * Add make rule to install ethersjs project deps * Update ci to prepare ehtersjs project * Add dockerfile for a node/golang CI executor Update CI to use new docker image, since we now need to run node and npm as part of our tests. * Run e2e coverage CI tests verbosely * Add debug * Add more debug * Change address format The IPV6 addresses returned from the geth node were not working on CI so we convert them to IPV4 addresses. * Update log message for clarity * Update node-golang dockerfile Add installed go binaries location to user's path * Update readme to specify a minimum go version This should mitigate the readme going out of date whenever we update go. * Default to single quotes in typescript * Flag to disable RPC compatibilty fields If the flag is set then the 'gasLimit' and 'baseFeePerGas' fields will not be returned on RPC blocks. * Split the tests under 2 describe headings One for tests with state one for tests without. This makes executing each batch of tests easier because we can just grep for the describe headings. * Document that js tests shoudn't be run standalone * Move ethersjs test project under e2e_test Just to further signify that it is not for standalone use. * Do not return block fields that cant be retreived If the state is missing for gasLimit or baseFeePerGas or if there is some other failure when retrieving them then do not add them as fields to the block. Preivously in some cases a default value would have been added to the block and returned. * Fix typos * Remove unused code Co-authored-by: Pasto <hbandura@gmail.com> Co-authored-by: Gaston Ponti <ponti@clabs.co>
Description
This PR aims to make our RPC API usable in the common case with Ethers.js, it tackles the following problems:
gasLimitfield was missing. ThegasLimitfield is now filled out on the block returned by the RPC API, note the field does not actually exist on the internal representation of the block.maxFeePerGasandmaxPriorityFeePerGasfields, because by the RPC API did not contain thebaseFeePerGasfield which Ethers.js uses to determine whether EIP-1559 style transactions are supported. ThebaseFeePerGasfield is now filled out on blocks returned by the RPC API, note the field dos not actually exist on the internal representation of the block.Discussion points
The
gasLimitfield is populated on the RPC block by getting the value from the contract state. If the state doesn't exist a default value (20M) is returned (this is pre-existing functionality for getting the gas limit) this means that blocks can exhibit inconsistent gas limit values depending on whether the node they are being retrieved from has the state for that block. Should we instead return no gas limit field in the case that there is no state?Similarly the
baseFeePerGasis populated on the RPC block by getting the value from the contract state but in contrast there is no default value so in the case that there is no state for the requested block the block returned from the RPC API will not have thebaseFeePerGasfield populated. This actually works ok for Ethers.js with respect to sending EIP-1559 style transactions because Ethers.js always requests the latest block to determine thebaseFeePerGas, however this could also lead to confusion for developers.Adding a flag to disable this change, currently there is no way to disable this change, this change doesn't seem to be a problem for our tooling, because all the monorepo e2e tests have passed in circleci, however it could cause issues for other users, it's hard to know how many may be affected. Should we add the flag?
Other changes
Updated the readme to specify a minimum go version rather than a current go version, this should relieve us from having to try and keep the readme in sync whenever we update go.
Tested
There's an e2e test that runs Ethers.js against our blockchain node to check that it works.
Related issues
Backwards compatibility
This is changing the RPC API response so Its not backwards compatible.
Release Notes
An additional two fields have been added to the block representation returned by the RPC api, they are:
gasLimitbaseFeePerGasBoth fields previously existed as parameters of the system held in contract state. The reason for this change is to improve compatibility with existing ethereum tooling specifically
Ethers.jswhich crashes if thegasLimitis not present on a block and is unable to send EIP-1559 transactions ifbaseFeePerGasis not present.Note that the internal block representation has not been modified so these new fields should not be taken into account in block hashing or signing operations. Since these fields are not part of the internal block representation they need to be fetched from state when the RPC call is made, if the state for these fields is missing then they will not be present on the block response. Non archive full nodes retain state for only the last 128 blocks, so when interacting with a non archive full node blocks more than 128 below the head block or blocks older than about 10 minutes will not have these fields present. If you require these fields on all blocks then you will need to make your RPC requests to an archive node.
A new flag has been added which allows disabling the addition of the new fields as a measure to help ease the transition to the new RPC block representation, the flag will likely be removed in a future release.