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

core: Refactor NewBlockchain -- Add blockChainConfig #28771

Closed

Conversation

makinje16
Copy link

@makinje16 makinje16 commented Jan 8, 2024

What

This PR modifies core.NewBlockChain and adds in a new blockChainConfig struct along with supporting types and methods.

Why

Issue #20280 mentions that the number of arguments to core.NewBlockChain had gotten out of hand and a config structure was desired. This issue was raised 5 years ago and the amount of refactoring needed has increased since.

Testing

  • Unit Tests Updated

Reference

@makinje16 makinje16 force-pushed the refactor-core-blockchain-config branch from 45768fb to b10358d Compare January 8, 2024 01:00
@makinje16 makinje16 marked this pull request as ready for review January 8, 2024 01:02
@makinje16 makinje16 changed the title Refactor Core.NewBlockchain -- Add blockChainConfig core: Refactor NewBlockchain -- Add blockChainConfig Jan 8, 2024
@makinje16 makinje16 force-pushed the refactor-core-blockchain-config branch from dd0d570 to 3bdb064 Compare January 8, 2024 14:18
@holiman
Copy link
Contributor

holiman commented Jan 30, 2024

	bcConfig := core.NewBlockChainConfig(
		core.WithCacheConfig(cache),
		core.WithGenesis(gspec),
		core.WithVmConfig(&vmcfg),
	)

In the scenario above, you basically define a new config. But, it doesn't make it a lot more ergonomical compared to if you had just made the config public. Then the corresponding code would look like this instead:

	bcConfig := core.BlockChainConfig{
		CacheConfig: cache,
		Genesis: gspec,
		VmConfig: &vmcfg,
	}

I don't really see any benefit of this pr.. ?

@holiman holiman closed this Jan 30, 2024
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.

None yet

2 participants