☂️ Create abstractions to make the wire serialization protocol exchangable.

[manoranjith]

Location

[wire], [pkg/io], [wire/net]

Motivation

Currently, wire messages in go-perun are encoded/decoded using a home-baked. When developing alternate implementations of the perun channel client, in a different programming language, the encoders and decoders have to be re-implemented in that language.

If we are able to exchange the custom format for standard ones like "protocol buffers", the messages can be defined once and the encoders/decoders can be generated for different programming languages. This will eliminate the overhead of implementing and maintaining the encoders/decoders in multiple languages.

However, this it is currently not possible to exchange custom format for a different one, as the necessary abstractions are missing. The scope of this task is to introduce the necessary abstractions.

Details

Abstractions necessary for making the wire serialization format exchangeable has been discussed and accepted in proposal#5.

This can be implemented in 6 steps, each involving one PR. This an umbrella issue to track the complete task.

• Step 1

  • Refactor Address,Asset, Data, Action types from (En|De)coder to binary.(Un)marshaler. Discussed in this comment and tracked in Refactor encoder/decoder interfaces for abstract types in the core packages of go-perun #257.
  • Move out pkg/io package to wire/perun. Move pkg/io to wire/perunio #285
  • Package pkg/io/test, which defines generic tests for encoders/decoders is moved to wire/test package. Merge perunio/test into wire/test #287

• Step 2 Add env serializer interface #297

  • Define EnvelopeSerializer interface.

  type EnvelopeSerializer interface {
      Encode(io.Writer, Envelope) error
      Decode(io.Reader) (Envelope, error)
  }
  

  • Add a unexported package level variable that holds an instance of EnvelopeSerializer and an exported function
    SetEnvelopeSerializer, for setting it. The function should panic, if invoked more than once.
  • Implement the EnvelopeSerializer for perunio serialization format. The implementation should be placed in new package :wire/perunio/serializer. Because, package wire/perunio cannot import package wire.
  • Add an init function in package wire/perunio/serializer, which sets the wire.enveloperSerializer to perunio envelope serializer.

  Remark: It would be nicer to have the EnvelopeSerializer implementation directly in package wire/perunio. However, this might require some refactoring (as described in step 7) and hence could be done later.

• Step 3 Add env serializer interface #297

  • Replace the wire.(Encode/Decode) methods with functions EncodeEnvelope and DecodeEvelope.
  • Update the Recv and Send on ioconn in the wire/net package to use the newly defined functions instead of wire.(Encode/Decode) methods.

• Step 4 Use UnmarshalBinary and MarshalBinary for converting to/from Binary form #298

  • Use (Un)MarshalBinary function instead of perunio.(En|De)code in places where data is to be converted to/from binary representation, but need not be written to the wire. In particular, see 75150e1 and 1dc0797.

• Step 5 (Can be done later)

  • Introduce an abstract type for wallet.Sig. Described and tracked in Add an abstract type for wallet.Sig #292.

• Step 6 (Can be done later)

  • Move the decoders map from the wire package to the wire/perun package.
  • In the wire package, decode envelopes instead of messages.

• Step 7 (Can be done later)
  From the client package,

  • Move the message definitions to the wire package and
  • Move the encoder/decoder implementations to the wire/perun package.
  • After these changes, merge wire/perunio/serializer package into wire/perunio. Because, now it would be possible for wire/perunio package to import wire package.

Notes:

1. Regarding persistence, we can continue to use the perunio protocol as that is used for wire serialization. This should continue to work without any additional changes unless we implement step 7. When we implement step 7, we could rethink how we should handle it.

2. Make ProposalID independent of perunio Mechanism for proposal ID generation #301

3. Export all relevant all message types 💥 Off-chain messages: Naming conventions and type export for wire serialization #302

[manoranjith]

While working on this change,

1. I moved "pkg/io" to "wire/perun".
2. I was moving interface definitions such as Encoder/Decoder from wire/perun to wire.

Then I noticed, these interfaces were used in three contexts:

1. Encode/Decode for sending via wire. (this is what we wanted to change).
2. Encode/Decode for persisting (we were aware of this).
3. For defining the types Asset, App and Action in a channel.

In the third case, the encoding/decoding will be implemented by the app backend and not the wire backend ?
How should we handle this ?

1. Can we define "Encoder/Decoder" interfaces in that channel package, which can handle this ? This will be in addition to Encoder/Decoder interfaces defined in wire package.
2. Can define update the interface definitions of App, Data and Action such that they directly define the Encode method (as shown below):

// Current definition
type Action = perunio.Encoder

// Proposed
type Action interface {
    Encode(io.Writer) error
}

@sebastianst @matthiasgeihs What do you think ?

[sebastianst]

The interfaces are still public, why do the current definitions of Asset, App and Action become problematic if they depend on wire.Encode/Decode?

[manoranjith]

Because, the documentation of Data says it will be decoded by the App Backend and that of Action says it will be decoded by the action app backend.

Which means, the encoding/decoding implementations for these packages are not defined by the wire protocol, but by the app backend ?

However, as I see it from the high level perspective: there are two levels of encoding/decoding happening here. First: Encoding the data into an app-agnostic format (this should be done by the app backend), Second: Serializing the data for transferring it over the wire. Do you think we should use two different abstractions to address each of these aspects ?
@sebastianst

[sebastianst]

Yes I think we have to regard this as two different levels of en/decoding. It is app specific how to en/decode the data for itself, and the resulting/source []byte slice should then be embedded into the message en/decoding. So I think every wire message en/decoding implementation has to use these Asset and App en/decodings.

[manoranjith]

Yes, in that case should we re-define the interface like shown below ?
(same would apply for other types).

type App interface {
    BinaryMarshaler
    BinaryUnmarshaler
}

Where the BinaryMarshaler and BinaryUnMarshaler interfaces are for converting the app specific data to and from binary format (byte array).

Then, the wire serialization backend can deal with encoding/decoding the byte arrays without being concerned about the type of app/action/data.

@sebastianst

[manoranjith]

I have added a demo of how this will look like in the draft PR #242, commit 5ce5487.

• Previously, the encoding function for address performed two functions:
  marshalling the address type into binary form and serializing the binary
  form to be sent on the wire.

• Add a new method "MarshalBinary" which marshals the address into
  binary form and use it in encode function.

• When this change is properly applied, the MarshalBinary and
  UnmarshalBinary will replace the Encode, Decode and Bytes functions.

• The same can be extended for other data types such as Action and App.

@sebastianst @matthiasgeihs

[matthiasgeihs]

@manoranjith

OK, let me see if I understand correctly:

The problem you are having is that for some types, we use their Encode function for different purposes that should potentially have different implementations.

P1: Encoding onto the writer.
P2: Encoding as a channel state property.

Correct?

Now you are saying you wanted to move the Encoder definition from the channel package to the wire package. And then you identified some problems.

My question: Is it really a suitable approach to move the Encode definition? Maybe we need to step back and carefully think again how we want to achieve the goal of making the wire serialization work for new clients? Maybe in the end the easier approach is to document our encoding properly and then just implement it across all clients?

Anyhow, it seems like a complex topic which we need to dedicate some more time to to get it right. We can do that in one of our upcoming calls. I had a brief look at your draft PR: having encoding.BinaryMarshaler, encoding.BinaryUnmarshaler in addition to Encode on each of the respective interfaces unfortunately doesn't look very clean to me.

[manoranjith]

Yes, your understanding of the primary issue is correct. It is that for some types (Address, Action, Data), Encode does two things:

1. Convert the data into binary form (The implementation for this would depend on the blockchain or app backend).
2. Write the binary data to wire (The implementation for this would depend on wire serialization protocol being used).

[manoranjith]

To your question on both encoding.BinaryMarhsaler and Encode functions being present in the draft PR.

It is just a draft to show, how the two concerns stated above can be implemented and which standard interfaces of golang (binary.Marshaler, binary.Unmarshaler) we can use.

In the end, on some types (where encode function does two things stated above), binary.Mashaler and binary.Unmarshaler will replace Encode/Decode methods and serialization functionality will be moved to wire package. For the other types (where encode function does only serialization i.e writing binary data to wire), the encode/decode functions will be removed and the functionality will be moved to wire package.

@matthias

[sebastianst]

I skimmed through your comments and the commit in the PR. Form what I understand, I think it does make sense to make this distinction between 1. (blockchain-dependent data serialization) and 2. (wire serialization).

If I saw correctly, using BinaryUnmarshaller forced you to add additional NewAddress etc factories to the backend, because now, e.g. the App interface doesn't have DecodeData(io.Reader) (Data, error) any more, which directly returns a Data instance. So you first have to use the factory method to then unmarshal into it... not sure if this really improves things.

[manoranjith]

@sebastianst Regarding the NewAddress factories, we could also do it other way around: Instead of a NewAddress factory, provide a DecodeAddress method on the backend. However, it is idiomatic in go, to instantiate an object and unmarshall/decode using it. Because, this way, the implementation of decode is coupled only to the concerned type.

[manoranjith]

Points from discussion with @matthiasgeihs

Currently, the Encoder/Decoder interfaces does two things (blockchain-dependent data-serialization) and (wire serialization). Hence, we need to refactor by replacing it with encoding.BinaryMarshaler/encoding.BinaryUnmarshal that does blockchain dependent data-serialization. Since, Binarymarshaler would output a byte array, it can be directly serialized/de-serialized by the wire package without any additional information.

After this refactor, it should be easier to create abstractions for the wire serialization. Hence, we could take up this task after the refactor.

[matthiasgeihs]

Update after meeting on Jan 11

• Postpone step 2 - 4.

• Replace Step 5 and 6 with the following:

Introduce wire.Encoder

1. In package wire, add Encoder interface.

type Encoder interface {
	Encode(io.Writer, *Envelope) error
	Decode(io.Reader) (*Envelope, error)
}

Also add method wireSetEncoder for setting the encoder globally, and methods wireEncodeEnvelope and wire.DecodeEnvelope. An example is given in b5f9f35.

2. Implement Encoder for perunio.
   In package wire/perunio, create an implementation of interface wire.Encoder that uses the perunio encoding.
   An example is given in b5f9f35.
   Remark: Currently, the perun encoding logic is contained in wire/perunio, but the Encoder implementation is in a sub package. It might be nicer to have the encoder implementation directly in package wire/perunio (or maybe even rename to something else, like wire/perun or wire/default), and maybe move the Perun encoding logic back to pkg/io. However, this might require some refactoring and could also be done in a later issue.

3. Change net.ioConn to use wire.EncodeEnvelope and wire.DecodeEnvelope.
   An example is given in b5f9f35.

The following is to be considered as well:

• Use (Un)MarshalBinary where applicable.
  • 75150e1
  • 1dc0797
• How do we derive ProposalID? Problem: Currently it depends on perunio.
  • For reference, have a look at 8bc3772.
• Which encoding do we use for persistence?

@manoranjith Please check my additions and modify your issue description as necessary.

[manoranjith]

@matthiasgeihs I have updated the issue description. Hope, I have covered all the points. Feel free to make suggestions/updates, if you find that I have missed anything.

[matthiasgeihs]

all tasks done