Improve createSnowChain readability #4195
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the createSnowmanChain method in the chain manager to improve code readability by extracting its dense implementation into multiple focused helper functions. The refactoring breaks down the monolithic chain creation process into logical components like database creation, message sender setup, peer tracking, VM initialization, and engine creation.
Key Changes
- Extracted database creation logic into
createSnowmanDBs - Separated message sender initialization into
createSnowmanMessageSender - Split peer tracking setup into
createSnowmanTrackedPeersandcreateSnowmanPeerTracker - Isolated VM creation logic into
createSnowmanVMs - Created dedicated engine setup in
createSnowmanEngine - Added P-Chain state setup helper
maybeSetupPChainState
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
chains/manager.go
Outdated
| // createSnowmanEngine creates a Snowman engine with the provided context, VM, and other parameters. | ||
| // It sets up the consensus engine, bootstrapper, and state syncer. | ||
| // It also handles setting up the P-Chain if necessary. | ||
| func (m *manager) createSnowmanEngine(ctx *snow.ConsensusContext, vm block.ChainVM, nonVerifyingParseFunc block.ParseFunc, sb subnets.Subnet, beacons validators.Manager, messageSender common.Sender, bootstrappingDB *prefixdb.Database, vdrs validators.Manager, connectedValidators tracker.Peers, peerTracker *p2p.PeerTracker, halter common.Halter) (*handler.Engine, error) { |
There was a problem hiding this comment.
This function has 11 parameters, which makes it difficult to read and maintain. Consider grouping related parameters into a struct (e.g., EngineConfig) to improve readability and reduce the chance of parameter ordering errors.
Copilot uses AI. Check for mistakes.
chains/manager.go
Outdated
|
|
||
| func (m *manager) createSnowmanVMs(ctx *snow.ConsensusContext, vm block.ChainVM, db *prefixdb.Database, primaryAlias string, genesisData []byte, messageSender common.Sender, fxs []*common.Fx) (*block.ChangeNotifier, *proposervm.VM, error) { |
There was a problem hiding this comment.
This function has 7 parameters, which is approaching the threshold for maintainability concerns. Consider grouping related parameters into a configuration struct to improve readability.
Copilot uses AI. Check for mistakes.
Signed-off-by: Sam Liokumovich <[email protected]>
| func (m *manager) IsBootstrapped(id ids.ID) bool { | ||
| m.chainsLock.Lock() | ||
| chain, exists := m.chains[id] | ||
| m.chainsLock.Unlock() | ||
| if !exists { | ||
| return false | ||
| } | ||
|
|
||
| return chain.Context().State.Get().State == snow.NormalOp | ||
| } | ||
|
|
||
| func (m *manager) registerBootstrappedHealthChecks() error { |
There was a problem hiding this comment.
This diff contains untouched but moved code hunks - which makes this diff harder to review. I'm open to us having helper functions but these code moves look unrelated to me - could we unstage these?
Why this should be merged
Only stylistic changes to
createSnowmanChainof the chain manager.createSnowmanChaininitializes theVM,Handlerand sets other components required to create a snowman chain. It was overly dense so I just separated it out into many helper functions to make it easier to digest. Very open to addition style suggestions, to make it even more understandable!How this works
How this was tested
Need to be documented in RELEASES.md?