Skip to content
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 baseFeePerGas to headers sent via EthSubscribe #2060

Merged
merged 2 commits into from
Apr 12, 2023

Conversation

karlb
Copy link
Contributor

@karlb karlb commented Apr 4, 2023

Description

Adding the baseFeePerGas to blocks returned by EthSubscribe makes the results more consistent with the data returned on Ethereum and via other RPC-Endpoints on Celo. And it is useful!

Once we add the baseFeePerGas to the header, this implementation is not needed anymore. So this change is only temporary.

I did not check for RPCEthCompatibility since this is not done for baseFeePerGas in the normal JSON-RPC responses, either.

Other changes

  • Enabled websockets for mycelo

Tested

Tested against mycelo with the following code

{
  "name": "subs",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "web3": "^1.9.0",
    "web3-providers-http": "^1.9.0"
  }
}
const Web3WsProvider = require('web3-providers-ws');
var provider = new Web3WsProvider('ws://localhost:9545');

const Web3 = require('web3');
const web3 = new Web3(provider)

async function wrapper(){
    const subs = web3.eth.subscribe('newBlockHeaders', function(error, result) {
      if (!error)
          console.log(result);
      else
          console.log(error);
    });
}

wrapper()

Related issues

Backwards compatibility

Adding a field to the response should not break sanely implemented consumers. IMO it is acceptable to consider this backwards compatible, although it is not true in a strict sense.

Makes it easier to test websocket specific operations (e.g.
subscriptions) and I don't see a reason not to turn in on.
@github-actions
Copy link

github-actions bot commented Apr 4, 2023

5774 passed, 1 failed, 46 skipped

Test failures:
  TestTransactionIndices: core
    blockchain_test.go:1944: Oldest indexded block mismatch, want 100, have 67
This test report was produced by the test-summary action.  Made with ❤️ in Cambridge.

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 4969b78

coverage: 49.3% of statements across all listed packages
coverage:  61.6% of statements in consensus/istanbul
coverage:  37.5% of statements in consensus/istanbul/announce
coverage:  54.5% 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:  64.9% 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/random

@karlb karlb force-pushed the karlb/basefee-in-header-subs branch from 49c9279 to a0731a2 Compare April 4, 2023 16:03
@karlb karlb marked this pull request as ready for review April 5, 2023 08:00
@palango palango requested a review from gastonponti April 5, 2023 10:10
@palango
Copy link
Contributor

palango commented Apr 5, 2023

Looks good to me, as a temporary measure until we add it to the header. But let's have @gastonponti have a look as well.

Copy link
Contributor

@gastonponti gastonponti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@karlb karlb merged commit 4969b78 into master Apr 12, 2023
@karlb karlb deleted the karlb/basefee-in-header-subs branch April 12, 2023 15:26
palango pushed a commit that referenced this pull request Apr 13, 2023
* mycelo: enable websockets

Makes it easier to test websocket specific operations (e.g.
subscriptions) and I don't see a reason not to turn in on.

* Add baseFeePerGas to headers via EthSubscribe

Closes celo-org/celo-blockchain-planning#59
palango pushed a commit that referenced this pull request Apr 13, 2023
* mycelo: enable websockets

Makes it easier to test websocket specific operations (e.g.
subscriptions) and I don't see a reason not to turn in on.

* Add baseFeePerGas to headers via EthSubscribe

Closes celo-org/celo-blockchain-planning#59
@palango palango mentioned this pull request Apr 13, 2023
gastonponti pushed a commit that referenced this pull request Apr 17, 2023
* mycelo: enable websockets

Makes it easier to test websocket specific operations (e.g.
subscriptions) and I don't see a reason not to turn in on.

* Add baseFeePerGas to headers via EthSubscribe

Closes celo-org/celo-blockchain-planning#59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants