Skip to content

les: do not directly import package eth from les#22154

Closed
zsfelfoldi wants to merge 5 commits into
ethereum:masterfrom
zsfelfoldi:remove_eth_import
Closed

les: do not directly import package eth from les#22154
zsfelfoldi wants to merge 5 commits into
ethereum:masterfrom
zsfelfoldi:remove_eth_import

Conversation

@zsfelfoldi
Copy link
Copy Markdown
Contributor

This PR does some refactoring in order to remove all imports of package eth from package les (except for api_test which is an end-to-end test). Dependence on the eth package was ugly anyway but the current reason is that eth package imports the tracer which imports duktape which cannot be compiled with CGO and therefore go-fuzz could not fuzz anything in package les.

  • eth.Config is moved to package eth/protocols/eth (is that a good place for the config?)
  • eth.NewBloomIndexer is moved to package core (the rest of the indexer logic is there anyway)
  • eth.CreateConsensusEngine is passed as a function parameter to les.New

Comment thread les/client.go Outdated

// New creates an instance of the light client.
func New(stack *node.Node, config *eth.Config) (*LightEthereum, error) {
func New(stack *node.Node, config *eth.Config, cc consensusCreatorFn) (*LightEthereum, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's use something a bit more friendly, perhaps

Suggested change
func New(stack *node.Node, config *eth.Config, cc consensusCreatorFn) (*LightEthereum, error) {
func New(stack *node.Node, config *eth.Config, mkEngineFn consensusCreatorFn) (*LightEthereum, error) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Jan 19, 2021

The config should probably live in a 'data package' that's shared between les and eth.
The package could be eth/ethconfig. This could also be the place for things like CreateConsensusEngine.

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