add RegistryFactory and deprecate global action, auth and output registries in VMs #1624
Conversation
tsachiherman
requested review from
aaronbuchwald and
joshua-kim
as code owners
chain/dependencies.go
Outdated
| @@ -287,3 +287,6 @@ type AuthFactory interface { | |||
| MaxUnits() (bandwidth uint64, compute uint64) | |||
| Address() codec.Address | |||
| } | |||
|
|
|||
| // RegistryFactory is the factory function, provided to the VM initializer that provides the registries for teh actions, auth and output. | |||
| type RegistryFactory func() (actionRegistry ActionRegistry, authRegistry AuthRegistry, outputRegistry OutputRegistry) | |||
There was a problem hiding this comment.
Would it be better to have a struct here?
type Registry struct {
action ActionRegistry
auth AuthRegistry
output OutputRegistry
}| type RegistryFactory func() (actionRegistry ActionRegistry, authRegistry AuthRegistry, outputRegistry OutputRegistry) | |
| type RegistryFactory func() Registry |
I'm not sure if we plan on extending this, but I think backwards-compatible changes would be easier with a struct.
There was a problem hiding this comment.
sounds good to me.
chain/dependencies.go
Outdated
| type Registry struct { | ||
| Action ActionRegistry | ||
| Auth AuthRegistry | ||
| Output OutputRegistry | ||
| } | ||
|
|
||
| // RegistryFactory is the factory function, provided to the VM initializer that provides the registries for the actions, auth and output. | ||
| type RegistryFactory func() Registry |
There was a problem hiding this comment.
Can we change from an interface returning a struct of all of these types to an interface that supports ActionRegistry(), AuthRegistry(), and OutputRegistry() individually?
We can have a struct that implements that interface and takes the exact same fields, that way we can pass it around more easily and call a single function on the interface to get the value we care about.
This will also make it more ergonomic when we have a type that needs only the one of those values. The callee can then declare its own dependency and the caller can pass in the existing interface.
vm/registry.go
Outdated
| type Registry struct { | ||
| actionRegistry chain.ActionRegistry | ||
| authRegistry chain.AuthRegistry | ||
| outputRegistry chain.OutputRegistry | ||
| } | ||
|
|
||
| func NewRegistry(action chain.ActionRegistry, auth chain.AuthRegistry, output chain.OutputRegistry) *Registry { |
There was a problem hiding this comment.
Could we define the struct implementing next to the interface in the chain package?
vm/vm.go
Outdated
| Registry: *NewRegistry( | ||
| registry.ActionRegistry(), | ||
| registry.AuthRegistry(), | ||
| registry.OutputRegistry()), |
There was a problem hiding this comment.
Why create a registry from the existing registry? Can we just set the registry to the chain.Registry value that was passed in?
| type Registry interface { | ||
| ActionRegistry() *codec.TypeParser[Action] | ||
| AuthRegistry() *codec.TypeParser[Auth] | ||
| OutputRegistry() *codec.TypeParser[codec.Typed] | ||
| } |
There was a problem hiding this comment.
- Why do we need this interface? It feels like this could be a simple struct that just wraps the individual codecs instead of an interface
- Why even couple the codecs at all w/ a wrapper type instead of passing around each codec individually?
| @@ -67,6 +66,8 @@ const ( | |||
| ) | |||
|
|
|||
| type VM struct { | |||
| chain.Registry | |||
There was a problem hiding this comment.
nit: Don't embed this type
| func (vm *VM) ActionCodec() *codec.TypeParser[chain.Action] { | ||
| return vm.Registry.ActionRegistry() | ||
| } | ||
|
|
||
| func (vm *VM) AuthCodec() *codec.TypeParser[chain.Auth] { | ||
| return vm.Registry.AuthRegistry() | ||
| } | ||
|
|
||
| func (vm *VM) OutputCodec() *codec.TypeParser[codec.Typed] { | ||
| return vm.Registry.OutputRegistry() | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: undo the move in this file
What ?
add RegistryFactory and deprecate global action, auth and output registries in VMs
Why ?
Avoid creation of global registries and pass these along the call stack as part of the server context.