Request: change cbg.CBOR(Un)marshaler interface to operate over byte slices

[ZenGround0]

I've brought this up in other channels but want to record it here for consideration.

The new interface would look like

type CBORMarshaler interface {
    MarshalCBOR() ([]byte, error)
}
type CBORUnmarshaler interface {
    UnmarshalCBOR([]byte) error
}

A byte slice oriented marshaling interface has good and bad tradeoffs related to the reader/writer design. Among the bad I see memory efficiency and the difficulty in changing the current interface used across several dependent packages.

The strongest reason for changing the interface and the reason I suggest this change is for interoperability with the go ecosystem. If you take a tour through the golang stdlib encoding package tree a pattern emerges: all Marshal interfaces operate on byte slices with roughly the above function signatures

Marshalers:

• text and binary marshalers
• asn1
• json
• xml

The pretty cool golang cbor library we're using in go-filecoin reasonably decided to look for custom marshaler functions with the signatures written above. Given the pattern set by the standard library I think that this library made the best choice, and I imagine other external code will also make the same assumption that the above interface is the best guess at a canonical cbor marshaling interface.

It would be awesome if cbg decided to switch to the same pattern. At the very least it would unlock awesome default interop between all of cbg's consumer package data types and go-filecoin. Thinking further out it would make cbg a friendlier package to the broader go ecosystem.

[hannahhoward]

One suggestion: if the concern is interop, I think we could just have cbor-gen output both, since the byte slice is simply a special case of a reader:

type CBORMarshaler interface {
   MarshalCBOR() (io.Reader, error)
   MarshalCBORBytes() ([]byte, error)
}
type CBORUnmarshalerBytes interface {
    UnmarshalCBOR(io.Reader) error
    UnmarshalCBORBytes([]byte) error
}

type Foo struct {
....
}

func (f * Foo) MarshalCBOR() (io.Reader, error) {
...
}

func (f * Foo) MarshalCBORBytes() ([]bytes, error) {
    r, err := f.MarshalCBOR()
    if err != nil { 
       return nil, err
    }
    return ioutil.ReadAll(r)
}

func (f * Foo) UnmarshalCBOR(io.Reader) error {
...
}

func (f *Foo) UnmarshalCBORBytes(buf []byte) {
   r := bytes.NewReader(buf)
   return f.UnmarshalCBOR(r)
}

There are probably a couple errors in the above, but it can definitely be written.

[ZenGround0]

The above suggestion is close but doesn't fix the problem. Adding Bytes to the name of the byte slice interface methods breaks the interop properties described above. But if you called the byte slice oriented methods (Un)MarshalCBOR and the reader oriented ones (Un)MarshalCBORStream that would fix the problem.

This means the suggested change must be a breaking change.

[hannahhoward]

I don't think the breaking change would be that bad -- especially since it's code gen'd in most places and is a find/replace in others. But I leave that to the maintainers of cbor-gen

[whyrusleeping]

I wrote up some notes on byte slice interfaces here: #20

[whyrusleeping]

also, to answer the question of "why not match the 'standard' go interfaces here?", short answer is that the standard go interfaces don't allow you to avoid extraneous allocations. A requirement for any truly performant marshaler is that it let the caller pass in the buffer to marshal into