Refactor controller to use empty interface instead of marshaled strings

[JimLarson]

What is the Problem Being Solved?

The current Agoric-specific Cosmos modules (x/lien, x/swingset, x/vbank, x/vibc) are endowed with a "controller" reference for communication to/from Javascript. The controller API accepts and receives strings which hold marshaled JSON text, tying these modules to JSON as an interface.

We'd like to refactor the JSON marshaling to allow these modules to be more JSON-agnostic and Golang-idiomatic, and thus potentially more efficient and widely useful.

Description of the Design

The controller API that a modules sees for sending to JS is callToController func(sdk.Context, string) (string, error), where the string is JSON, typically marshaled from a JSON-annotated structure suitable for encoding/json.Marshal().

Similarly, the API for receiving from JS is Receive(*vm.ControllerContext, string) (string, error), again where the strings are JSON. (The *vm.ControllerContext is a no-op wrapper around a sdk.Context and could be simply refactored away.)

The proposal is to replace the use of string with interface{}, where the interface{} dynamically matches the types specified in encoding/json.Marshal() or Unmarshal() as appropriate. The actual marshaling and unmarshaling will be handled in the controller implementation.

The proposal for callToController is a usage like err := keeper.callToController(ctx, &reqMsg, &replMsg), where the called function handles the marshaling of the request and unmarshaling of the reply.

The Receive() equivalent isn't straightforward, as the caller must handle the unmarshaling - what's the message type? We could either add another method to the vm.PortHandler interface: NewMsg() interface{} which would return a new, blank message suitable for marshaling into, or pass an unmarshaling function in: Receive(sdk.Context, func(interface{}) error) (interface{}, error).

Security Considerations

None foreseen.

Test Plan

Refactor of existing code, so

[JimLarson]

@michaelfig please correct or elaborate as necessary.

[michaelfig]

The Receive() equivalent isn't straightforward, as the caller must handle the unmarshaling - what's the message type?

Ah, okay, after looking at the code I see what you mean.

We could either add another method to the vm.PortHandler interface: NewMsg() interface{} which would return a new, blank message suitable for marshaling into, or pass an unmarshaling function in: Receive(sdk.Context, func(interface{}) error) (interface{}, error).

I think these two approaches break down when we consider that a given PortHandler may receive more than one message type.

So, I think my preferred mechanism (though maybe it should be spelt differently) is:

type PortHandler interface {
  // Receive and type-dispatch a message from the controller.
  Receive(sdk.Context, interface{}) (interface{}, error)
  // Marshal converts a Go type to a byte slice.
  Marshal(interface{}) ([]byte, error)
  // Unmarshal converts a byte slice to a Go type.
  Unmarshal([]byte) (interface{}, error)
}

I think it's fine for the port handler to be in control of the serialisation for that port. I think the Unmarshal method would use an internal registry to demux []byte to different types, like it would if we moved to protobuf for these messages.

Also, switching from string to []byte and encapsulating that in the PortHandler is probably the idiomatic thing to do. We should update our JS controller for []byte as well.

[JimLarson]

Begin rant. Grumble. I think the Golang encoding/json package makes a fundamental semantic error by marshaling to []byte instead of string, thereby hard-wiring the utf8 character encoding into the translation, whereas RFC 7159 makes it clear that utf16 and utf32 are also supported.

See this history to see similar confusion in a base64 library and the resulting simplification when text is represented as string and character encoding is specified by explicit composition.

In any case, I think it's fair to sidestep this issue and just say that our encoding isn't "JSON" but "JSON∘utf8". End rant.

[JimLarson]

So by making a convenient entry point that reacts to Go data, it's worth being clear about how this feature should and shouldn't be used, relative to other options like:

• a normal method call on a keeper;
• an ABCI message to a module;
• a query on a module.

Like an ABCI message, the controller messages may update state and must be deterministic, and there is a routing mechanism to locate the right PortHandler. Unlike ABCI messages, there is no "check" operation.

It might be a best practice to have read-only operations have an equivalent query, both mapping to the same underlying keeper method.

Question: would we ever want to do a module-to-module call to the Receive() method instead of a normal call to a keeper?

[michaelfig]

Question: would we ever want to do a module-to-module call to the Receive() method instead of a normal call to a keeper?

I don't think so.

[JimLarson]

Discussion today with @michaelfig

• For consistency, we should be inspired by the in-memory transport of Tendermint (which seems to be based on structs generated from protos, which involve uninterpreted []bytes for unknown structures.
• We could write a controller based on protocol buffers, with annotation giving translation to JSON
  • Alternative: could at least specify our JSON messages via JSON schema. Investigate JSON schema integration with protobuf.
  • JSON schema validation can be costly - probably unnecessary.
• How is a controller distinguished from other module mechanisms?
  • It's a privileged communication channel - no authentication required.
  • It provides the policy for policy-less modules.
  • Perhaps a "policy module" would be a better approach?
    • One destination for bridge messages.
    • Makes modules useful outside of Swingset.
• Need to look at current Cosmos development plans, as similar mechanisms are in the works.

[JimLarson]

Notes from discussion with @michaelfig

• Settling on the idea of a separate policy module which communicates with other keepers via normal Go method calls.
  • x/controller module to replace vm
• Keeper reference graph would be cyclic, e.g. x/controller <--> x/vbank
  • Jim suggests splitting controller module into downcall and upcall modules to avoid cycles.
  • Michael would rather keep it as one module. Cyclic links can be created outside of keeper constructors, e.g. vbankKeeper.SetController() and controllerKeeper.SetVBank(). This would make an extensible API, vs piling more arguments into the keeper constructors.
• Controller module would have semantic knowledge of all bridge messages and translate to and from keeper method calls.
  • One bridge downcall could become multiple keeper method calls, e.g. to implement promise chaining.
• It would be nice to factor storage out of x/swingset into a new x/storage.

[michaelfig]

Looks like a plan!

Btw, I'd suggest other keepers could receive the controller keeper in their constructor for upcalls. Just the downcall targets would use controllerKeeper.Set*.

Thanks.

[ivanlei]

No longer plan of record.