Who should decide how to encode a type, the type itself or the message that contains it?

[arnauorriols]

There are 2 common approaches to defining the encoding/decoding of a type.

The first, and straightforward one is to define the encoding of a type as an intrinsic property of the type itself. This approach will see the encoding implemented in the very same module where the type is defined, alongside its other intrinsic properties and logic. For example, the serde::Deserialize and serde::Serialize trait implementations, or the ContentWrap or ContentUnwrap in the case of DDML.

This approach works great in simple scenarios where there's only one way to encode the types, with a single port through which they need to be serialized, and it helps getting the knowledge about the types encapsulated into their own module. However, it can get problematic quickly in the following scenarios:

1. There are different APIs that require different encoding for the same type (even if using the same encoding scheme)
2. A type is part of different greater structures, each of which needs to encode the type in different ways

In these cases, one needs to define an encoding specifically for each target port. This could still be defined in the type's module, but it would obviously derive into an improper coupling between the external port interfaces and the core domain type.

I want to expose in this discussion that the second case is what is happening with Identifier. Identifier is a low-level type that represents the public identifier of a subscriber. This type is used within all the messages of Streams. In contrast with other types used within the messages, this one implements its own ContentWrap, ContentUnwrap and ContentSizeof, and the messages delegate the wrapping and unwrapping of this type to the methods Identifier::wrap(), Identifier::unwrap() and Identifier::sizeof(). The encoding of this type uses the DDML command mask to encode the identifier to the byte stream. However, it is not logically correct that it is always masked; in keyloads and sequence messages it should actually be absorbed.

In my opinion, encoding should generally be implemented in the outer-most, port specific structure. Even if this might seem to generate a bit of duplication, I defend that this duplication is in fact not duplication but actually happy coincidence. Rushing to DRY up the code by deduplicating encoding often results in wrong abstractions and the consequently messy workarounds. In the case of Streams, I suggest getting rid of the implementation of ContentWrap (etc) for Identifier, and implementing the pattern matching of the different cases of Identifier in the wrap (etc) method of each message type.

[DyrellC]

Although I generally agree with the premise that it doesn't make sense to mask Identifier in every instance, I think there's something to be said about the way the DRY approach simplifies future development when including new Identifier types. Although the topic of this conversation is centered around Identifier wrap/unwrap processes, it has direct implications for signature and key exchange logic as well, and if we pull the logic back out into the message wrapping/unwrapping it's not just a small amount of duplication (as is seen from the DID related signature implementations). It is a non-negligible addition to include these matching processes in every message type, and I would argue actually decreases readability substantially by not including an adequate separation of responsibilities.

For the Identifier wrap/unwrap it's a simple enough change, but for signature and key exchange logic it can get very verbose very quickly, and instead of changing the logic in one place (like in the trait implementation for UserIdentity) we have to be mindful of making duplicated changes to several message types each time, which seems like a maintenance nightmare.

[arnauorriols]

I would argue that DRY is not about removing syntactic duplication but semantic duplication. It's not merely about removing character duplication, it's about identifying intrinsic knowledge and abstract it into a more semantically meaningful interface so that upper layers can reuse this interface. This is a more nuanced process, and getting it wrong has serious consequences.

You are completely right in identifying duplication in the UserIdentity operations. I'm not arguing against abstracting away the signing and key-exchanging actions into UserIdentity::sign() and UserIdentity::key_exchange() methods (etc). This is indeed intrinsic knowledge that must be abstracted away, the messages don't need to know how a UserIdentity generates a [u8; 64] signature given a hash and a private key. However, it is responsibility of the messages (and not UserIdentity) how this resulting [u8; 64] is serialized (wrapped) within the message.

TL;DR: the UserIdentity methods should remain in a high level of abstraction, while the specific conversion from the output of the methods to DDML serialization is still responsibility of each message.

[DyrellC]

I see the point you're making, extracting the actual signing and key exchange logic out, but there's also the logic that comes along with that in the nested ddml operations that are more specific to the implementation (like wrapping did information for the method for example to then be used to conduct those operations). Are you proposing that that be the part that essentially would need to be duplicated in the Messages operations? Because that could get decently messy, especially if we add additional Identifier types.

Although with that being said, if we can find a nice way to wrap/unwrap the Identifier::DID that includes the arbitrarily lengthed DID along with the network and method details, then that part of the nesting wouldn't be necessary and my concerns would be dispelled, but I'm still grappling with that concept. 🤔

[DyrellC]

Another thing worth noting is: the changes you proposed to the Idenitifer::DID to use the full url (and parse from that url for transport related functionality) rather than just the method id introduces a more complicated approach to the wrapping and unwrapping of the Identifier itself, which would cloud the simplicity of that duplication effort for even just the Identifier process.

[DyrellC]

I'm sure a happy medium exists somewhere between the two propositions, but it'll take a bit of discussion to find it.

[kwek20]

If were talking about future corrections to the code base (Which can be done without breaking anything) this is great starting point for a discussion. I do agree that its weird that identifier is the one off handling ddml like this. Although i have to add to this that permissions would also go the same route (vs what you propose having a match in the required messages for a permissions "type") So this is not a one-off case, and as dyrell said it would lead to A LOT of syntactic duplication.

That said; think we should find something more maintainable, copy/paste around the message isnt going to do the trick.

For example, we could just use a combination of as_bytes/to_bytes and a type/number attached to the enum. That would make i think the implementation generic already? Or force each identifier to implement Size. I think theres many options.

Alternatively, we can still extract the match cases to a static method somewhere so theres very little duplication