Add a VM with Contract support #1493
Conversation
|
Could we rename from Any thoughts @richardpringle @samliok ? |
@aaronbuchwald, we could rename it, but we should be deliberate. One issue with |
Co-authored-by: aaronbuchwald <aaron.buchwald56@gmail.com> Signed-off-by: David Boehm <91908103+dboehm-avalabs@users.noreply.github.com>
richardpringle
force-pushed
the
ProgramsVM
branch
from
b300235
to
abf209b
Compare
There was a problem hiding this comment.
LGTM to merge in, so we can follow up on the comments in this review.
I do think we should rename the example from vmwithcontracts to contractvm.
We may want people to use contracts as a part of their VM and not just the only way to interact with their VM, but this example is focused on contracts regardless. Either way - we need a better name than vmwithcontracts
| func (t *Call) StateKeys(_ codec.Address, _ ids.ID) state.Keys { | ||
| result := state.Keys{} | ||
| for _, stateKeyPermission := range t.SpecifiedStateKeys { | ||
| result.Add(stateKeyPermission.Key, stateKeyPermission.Permission) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Could we add a TODO with the details and let the TODO bot auto-create the issue?
|
|
||
| r *runtime.WasmRuntime |
There was a problem hiding this comment.
(No changes necessary) We should treat this as an anti-pattern. Action structs should aim to be stateless, I assume this is needed to re-use the runtime, is there another way to do this?
| func TestCallAction(t *testing.T) { | ||
| ts := tstate.New(1) | ||
| addr := codec.CreateAddress(0, ids.GenerateTestID()) | ||
| tests := []chaintest.ActionTest{ |
There was a problem hiding this comment.
We should add real tests here
|
|
||
| var _ chain.Action = (*Deploy)(nil) | ||
|
|
||
| const MAXCREATIONSIZE = units.MiB |
There was a problem hiding this comment.
Nit: can we update this to MaxCreationSize
| } | ||
|
|
||
| func (*Deploy) StateKeysMaxChunks() []uint16 { | ||
| return []uint16{storage.BalanceChunks} |
There was a problem hiding this comment.
No, I'm not sure why this was marked as resolved
| return vm.NewOption(Namespace+"runtime", *runtime.NewConfig(), func(v *vm.VM, cfg runtime.Config) error { | ||
| wasmRuntime = runtime.NewRuntime(&cfg, v.Logger()) | ||
| return nil |
There was a problem hiding this comment.
Is this just using the option construct to set a global variable with params provided by the VM?
| } | ||
|
|
||
| func WithRuntime() vm.Option { | ||
| return vm.NewOption(Namespace+"runtime", *runtime.NewConfig(), func(v *vm.VM, cfg runtime.Config) error { |
There was a problem hiding this comment.
Can we change the name to just runtime instead of including the controller?
| // New returns a VM with the indexer, websocket, rpc, and external subscriber apis enabled. | ||
| func New(options ...vm.Option) (*vm.VM, error) { | ||
| opts := append([]vm.Option{ | ||
| indexer.With(), | ||
| ws.With(), | ||
| jsonrpc.With(), | ||
| With(), // Add Controller API | ||
| externalsubscriber.With(), | ||
| }, options...) | ||
|
|
||
| return NewWithOptions(opts...) | ||
| } | ||
|
|
||
| // NewWithOptions returns a VM with the specified options | ||
| func NewWithOptions(options ...vm.Option) (*vm.VM, error) { | ||
| opts := append([]vm.Option{ | ||
| WithRuntime(), | ||
| }, options...) | ||
| return vm.New( | ||
| consts.Version, | ||
| genesis.DefaultGenesisFactory{}, | ||
| &storage.StateManager{}, | ||
| Action, | ||
| Auth, | ||
| auth.Engines(), | ||
| opts..., | ||
| ) | ||
| } |
There was a problem hiding this comment.
Let's update to using defaultvm and doesn't this need to include both WithRuntime and With to include the API for this VM?
| "github.com/ava-labs/hypersdk/x/programs/runtime" | ||
| ) | ||
|
|
||
| // [accountStatePrefix] + [account] |
There was a problem hiding this comment.
This should be accountPrefix + account + accountStatePrefix
| programBytes []byte, | ||
| ) ([]byte, error) { | ||
| programID := ids.ID(sha256.Sum256(programBytes)) | ||
| key, _ := keys.Encode(ProgramsKey(programID[:]), len(programBytes)) |
There was a problem hiding this comment.
Can we be consistent between using keys.Encode internally to the storage helper functions vs. calling it on the result?
We'll need to pass in the full bytes to the above functions, so it has the length as well.
It seems like we're doing this correctly, but it's unnecessarily difficult to follow because ProgramsKey is exported and must be wrapped with keys.Encode where it has the full value it's going to store whereas StoreProgram calls keys.Encode internally
Create an example VM that has the ability to deploy and run wasm smart contracts. This VM is based on the morpheus vm, but adds contract support.