-
Notifications
You must be signed in to change notification settings - Fork 126
Add package level godoc for snow #2011
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
base: main
Are you sure you want to change the base?
Conversation
I think we can improve reading flow and format if we were to use Markdown supported by For example
|
snow/doc.go
Outdated
// Snow provides an adapter from the AvalancheGo [block.ChainVM] interface to provide a more ergonomic interface for chain developers. | ||
// The existing interface was largely defined for ease of use by the AvalancheGo consensus engine and as a single all access type for | ||
// anything required by AvalancheGo (APIs, health check, shutdown, etc.) | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what specific linter or rule that's coming from? I'd prefer to add the linter and start to follow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found this lint rule: https://staticcheck.dev/docs/checks/#ST1020
snow/doc.go
Outdated
// This package defines a simpler to use interface for use within the HyperSDK and to eventually iterate from | ||
// an initial small, well-defined public API backwards to more layers of customizability including building directly on top of this package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does not belong to the documentation, it describes internal development strategy rather than information about the package.
IMO documentation should focus on what the package currently does, how to use it, and what problems it solves. Future development plans would be better communicated in a separate roadmap document or project README, not in the package's API documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to:
// This package implements these tough to get right and tedious components and provides
// a slimmed down interface for VM developers to implement.
snow/doc.go
Outdated
// This package simplifies the existing ChainVM interface by implementing the [snowman.Block] interface and allowing VM developers to provide | ||
// generic type parameters for each possible state of a block within consensus (bytes, input/un-verified, output/verified, accepted). | ||
// Developers implement a single function to transition between each of these states with all of the information guaranteed to be available | ||
// at that point in time. | ||
// | ||
// For example, when a block is verified, its parent is guaranteed to have been verified successfully. However, it may not be accepted. Therefore, | ||
// chain developers implement `VerifyBlock` and receive the verified version of the parent (but not accepted type) and the input block to be verified: | ||
// | ||
// VerifyBlock( | ||
// ctx context.Context, | ||
// parent O, | ||
// block I, | ||
// ) (O, error) | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A combination of what you wrote with this should answer the "why snow" question, imo.
// # Snow egonomics (or something similar, this doesn't need to be h1)
// Snow simplifies VM development through a type-safe block state system
//
// Instead of managing block state machine, Snow introduces
// **generic** type parameters that represent each distinct block state:
//
// - B: Raw block bytes
// - I: Input/unverified blocks ready for processing
// - O: Output/verified blocks that passed validation
// - A: Accepted blocks committed to the chain
//
// This type system ensures you always have access to the appropriate block
// representation at each stage of consensus, preventing common errors and
// simplifying implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great call on breaking out the block types this way, incorporated this feedback with a bit more modification
snow/doc.go
Outdated
// Copyright (C) 2024, Ava Labs, Inc. All rights reserved. | ||
// See the file LICENSE for licensing terms. | ||
|
||
// Snow provides an adapter from the AvalancheGo [block.ChainVM] interface to provide a more ergonomic interface for chain developers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be too much but it'd be good to explain block.ChainVM:
ChainVM is the required interface that any Snowman-compatible blockchain must implement to work with Avalanche's consensus engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going with a bit shorter: (the interface AvalancheGo VM developers must implement)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm too close to it, but a long explanation of what block.ChainVM
is seems like overkill or re-summarizing the same information to me. Tried to keep it short, let me know if you have another suggestion!
snow/doc.go
Outdated
// Further, the package provides two functions [chain.Initialize] and [chain.SetConsensusIndex] as the effective "entrypoint" of the VM. | ||
// This needs to be split into two parameters, because the consensus index depends on a fully initialized version of the chain. | ||
// This provides all of the information available to the VM at the time of initialization. See [Chain.Initialize] and | ||
// [Chain.SetConsensusIndex] for details on the parameters provided to the VM at initialization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Struggling to get these links to work within the same package based off these docs https://tip.golang.org/doc/comment#links ... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing chain.Initialize
and chain.SetConsensusIndex
w/o hyperlinks hurts me, but everything else looks great!
// For example, when a block is verified, its parent is guaranteed to have been verified successfully. However, it may not be accepted. Therefore, | ||
// chain developers implement `VerifyBlock` and receive the verified version of the parent (but not accepted type) and the input block to be verified: | ||
// | ||
// VerifyBlock( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: change to Output, Input
// to provide a more ergonomic interface to VM developers. | ||
// | ||
// The existing interface was largely defined for ease of use by the AvalancheGo consensus engine. As a result, | ||
// all responsibilities are combined into a single, all-access type for anything required by AvalancheGo (APIs, health check, shutdown, etc.). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a result, all responsibilities are combined into a single, all-access type for anything required by AvalancheGo (APIs, health check, shutdown, etc.).
Is this the problem with using the current block.ChainVM
interface? If so, can we make it so that the problem is evident? It seems that this is a statement rather than describing the actual problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the following:
This package implements these tough to get right and tedious components and provides a slimmed down interface for VM developers to implement.
is actually describing the problem.
verifiedSubs []event.Subscription[Output] | ||
rejectedSubs []event.Subscription[Output] | ||
acceptedSubs []event.Subscription[Accepted] | ||
preReadyAcceptedSubs []event.Subscription[Input] | ||
// preRejectedSubs handles rejections of I (Input) during/after state sync, before they reach O (Output) state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove references to the former I
generic name?
This PR adds a
doc.go
with a high level description of the snow package.