Separate snow package from snow vm refactor #1846
Conversation
| return &InputCovariantVM[I, O, A]{a.vm.covariantVM} | ||
| } | ||
|
|
||
| func (a *Application[I, O, A]) WithAcceptedSub(sub ...event.Subscription[A]) { |
There was a problem hiding this comment.
With* naming is usually reserved for when a type returns itself which also supports chaining, this is more analagous to a Set* function
| chainInput ChainInput, | ||
| makeChainIndex MakeChainIndexFunc[I, O, A], | ||
| app *Application[I, O, A], | ||
| ) (BlockChainIndex[I], error) |
There was a problem hiding this comment.
Why is Chain responsible for creating the index? Can't the snow VM implementation own this because it handles all blocks from consensus?
| ) (BlockChainIndex[I], error) | ||
| BuildBlock(ctx context.Context, parent O) (I, O, error) | ||
| ParseBlock(ctx context.Context, bytes []byte) (I, error) | ||
| Execute( |
There was a problem hiding this comment.
Should we call this Verify?
| "github.com/ava-labs/hypersdk/event" | ||
| ) | ||
|
|
||
| type Application[I Block, O Block, A Block] struct { |
There was a problem hiding this comment.
We could actually probably get rid of Application entirely and just merge this code into VM. If we want to hide stuff from Chain we can do that by un-exporting the corresponding functions
| AcceptBlock(ctx context.Context, acceptedParent A, outputBlock O) (A, error) | ||
| } | ||
|
|
||
| type VM[I Block, O Block, A Block] struct { |
There was a problem hiding this comment.
I think the relationship between VM, *CovariantVM, and Application are confusing, but this is my understanding of them - correct me if I'm wrong on any of these...
snow.VMis an adapter/wrapper pattern to make the hypervm compatible with avalanchego consensussnow.Applicationis meant to be a way to expose theVMwithout too many internal implementation details tosnow.Chainsnow.*CovariantVMexists because we wantChainto be able to see a concrete type instead of an opaque interfacesnow.Chainexists because we want developers to be able to customize a set of vm-level behavior
Some ideas to simplify the relationship between these types:
snow.VM+ the related consensus adapter types (e.gStatefulBlock) I don't think are relevant to the developer and bloat the package. I think we should move these tointernaltointernal/snow.*CovariantVMis awkward because it's purpose is to implement theVMwith type information for a better DX, but has a circular dependency with theVM, because*CovariantVMneeds a reference to information owned byVM(example, becauseVMand*CovariantVMshare the responsibility of implementing the "entire" VM, but divide the implementation between typed/untyped responsibilities. I think we can simplify by implementing the entire vm inCovariantVMand just wrapping it withsnow.VMto erase type information where needed to conform to avalanchego's interface, so thatsnow.VMis just a simple wrapper.- I also think that instead of having two separate covariant wrappers for the input/output blocks that we can just have a single covariant type like
Block{}that contain both the input/output type to simplify theChainDX.
| func PrefixBlockKey(height uint64) []byte { | ||
| k := make([]byte, 1+consts.Uint64Len) | ||
| k[0] = blockPrefix | ||
| binary.BigEndian.PutUint64(k[1:], height) | ||
| return k | ||
| } | ||
|
|
||
| func PrefixBlockIDHeightKey(id ids.ID) []byte { | ||
| k := make([]byte, 1+ids.IDLen) | ||
| k[0] = blockIDHeightPrefix | ||
| copy(k[1:], id[:]) | ||
| return k | ||
| } | ||
|
|
||
| func PrefixBlockHeightIDKey(height uint64) []byte { | ||
| k := make([]byte, 1+consts.Uint64Len) | ||
| k[0] = blockHeightIDPrefix | ||
| binary.BigEndian.PutUint64(k[1:], height) | ||
| return k | ||
| } |
There was a problem hiding this comment.
Should these be un-exported?
| } | ||
|
|
||
| func (c *ChainStore[T]) GetLastAcceptedHeight(_ context.Context) (uint64, error) { | ||
| lastAcceptedHeightBytes, err := c.db.Get([]byte{lastAcceptedByte}) |
There was a problem hiding this comment.
Seems like it might make sense to just make this byte array a var since we're frequently re-using it
| Register(name string, gatherer prometheus.Gatherer) error | ||
| } | ||
|
|
||
| type Context struct { |
There was a problem hiding this comment.
I generally dislike these Context patterns (we do this in avalanchego... but I still dislike snow.Context because I feel like it makes it too easy to pass around global state/dependencies that a type really doesn't care about. It's also unclear to me why some dependencies in Initialize exist/don't exist in snow.Context, like the database... I feel like we should be consistent). I would probably prefer us just passing dependencies around individually as parameters.
| func (c Config) Get(key string) ([]byte, bool) { | ||
| if val, ok := c[key]; ok { | ||
| return val, true | ||
| } | ||
| return nil, false | ||
| } | ||
|
|
||
| func GetConfig[T any](c Config, key string, defaultConfig T) (T, error) { | ||
| val, ok := c[key] | ||
| if !ok { | ||
| return defaultConfig, nil | ||
| } | ||
|
|
||
| var emptyConfig T | ||
| if err := json.Unmarshal(val, &defaultConfig); err != nil { | ||
| return emptyConfig, err | ||
| } | ||
| return defaultConfig, nil | ||
| } |
There was a problem hiding this comment.
Should we just unexport Get? I feel like we're not going to use it and will probably always be using GetConfig.
| MinFee uint64 `json:"minFee"` | ||
| } | ||
|
|
||
| func TestConfigC(t *testing.T) { |
There was a problem hiding this comment.
nit: Is this a typo in the test name *ConfigC?
This PR breaks out the snow package from the Snow VM refactor: #1831
Breaking this out to make it an additive change. You can see how this refactor impacts the HyperVM and MorpheusVM example implementation in the original PR.
The added
snowpackage implements the types required by the AvalancheGo consensus engine and decomposes them so that a VM developer overrides only the components relevant to their VM and implements the minimumChaininterface:This PR additionally adds unit tests that the
snowpackage correctly maintains the AvalancheGo consensus engine invariants and a fuzz test.