simplify wire format of our Simple server

[vinipsmaker]

[maidsafe-highfive]

r? @canndrew

(maidsafe_highfive has picked a reviewer for you, use r? to override)

[dirvine]

I wonder if this is OK, would it not need byteorder also if we are to encrypt all messages very soon, which may require that all get wrapped in an enclosing enum then this may make it harder? Not sure, but be good to clarify this.

[vinipsmaker]

I wonder if this is OK, would it not need byteorder also if we are to encrypt all messages very soon, which may require that all get wrapped in an enclosing enum then this may make it harder?

Can you elaborate on this? Maybe split your concerns into different sentences?

About byteorder, we only do byte order conversion when we have integers larger than 1 byte. This isn't the case here. Also, we had code like this before.

[dirvine]

Ah no problem. On the encryption issue, I feel that we will have a message method/struct/enum (probably enum) that accepts all message types but to prepare them for sending at the moment we serialise them. I have a new crate https://github.com/maidsafe/secure_serialisation to allow fully secured messages (authenticated public key encrypted) by replacing serialise with secure_serialise. This is to make it simple to add the security we require and assuredness a message came from who we wanted (instead of sending secrets for instance in stun). So this is to make it simple and as a close to a drop in as possible. It does require a precomputed key (which is our secret key and their public key) to be held. This precomputed key is more efficient than always using both keys, but is also good to keep code tidy (as we refer to this single key as opposed to wondering about secret vs public etc. in code. So a single key per peer if you like.
So with that in mind to keep a drop in replacement simple as possible it seems to make sense to have the messages using a single message enum with an impl that defines prepare_for_send or similar which may call just plain serialise at the moment but can be easily replaced with secure_serialisaiton::serialise or better secure_serialisation:: pre_computed_serialise to allow the security and validation of messages.
Hope that helps.

[canndrew]

I still don't see how the enum helps. pre_computed_serialise takes a T where T: Encodable. And [u8; 4] is encodable.

[dirvine]

What I mean is that this makes it receive a single message type which is an array, or with an enum receive a multitude of message types and parse to the correct type. The enum wraps a bunch of the types which allows us to have more message types per connection. Otherwise we try and parse to type1 then type2 etc. if you see what I mean. Also the enum allows us to implement a the methods we wish to do on send/rec messages.

[vinipsmaker]

Hope that helps.

Yes, it helped. Thanks.

So... do I just need to convert this byte sequence into an empty struct that implements rustc_serialize::Encodable?

[dirvine]

I think so, or if we say this connection can only ever get 1 message type then we can almost ignore the enum. It's more where we are trying to identify/dispatch message types we could receive an enum or identifier is required. IF there is a sinlge type then we can ignore this at the moment I think.

[vinipsmaker]

I think so, or if we say this connection can only ever get 1 message type then we can almost ignore the enum.

Sorry, but you said yes to my previous "suggestion", which by itself was already suggesting to ignore the enum (convert this byte sequence into an empty struct).

IF there is a sinlge type then we can ignore this at the moment I think.

Yes, I think there is only a single type for now. Does it mean I should convert to an empty struct or leave as is?

[dirvine]

If it's a single type I think just try and decode into that type is fine. Probably no need for identifier either if it's just a single thing as we will just echo regardless what hits us here.

[vinipsmaker]

So I think discussion is resolved and @canndrew can merge the PR.

[vinipsmaker]

I rebased the PR against latest master.

[vinipsmaker]

So I think discussion is resolved and @canndrew can merge the PR.

Tests passed. Just waiting for @canndrew now.

[canndrew]

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[canndrew]

Reviewed 1 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[canndrew]

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io