Refactor/messages crate

[bobozaur]

Refactor of messages crate.

Planned steps:

• Initially, development will be carried in a new crate, messages2.
• Once ready, the messages2 crate will be merged and we can start using it in aries-vcx, slowly replacing the old crate.
• After all the occurrences of the old messages crate are addressed, it will be removed.

Pending tasks:

• Module structure:

  • Move things used by messages_macros crate somewhere where they can be used with fully qualified paths
  • Move the error module inside message_type as these are the only custom errors that can arise
  • Move crate::msg_types::types::traits to crate::msg_types::traits.
  • Introduce a directory in crate::protocols to separate the message implementations from the traits.
  • Move Service from crate::protocols::common to crate::protocols::out_of_band, as it's only used there.
  • Decompose/organize the composite_message module
  • Move the HasKind trait out of delayed_serde module
  • Moved the MSG_TYPE contant to utils
  • Created misc module that contains stuff such as MimeType, the Nothing type, and the utils module.
  • Make types more accessible through re-exports

• Documentation:

  • Document messages with links to their respective RFCs
  • Add module level documentation where appropriate
  • Document the crate::msg_types::traits module contents.
  • Document MessageType
  • Document MsgWithType
  • Document Message and the way it composes/decomposes
  • Document the purpose of HasKind MessageWithKind trait
  • Document proc_macros
  • Document registry module
  • Document Actor Role enum
  • Document Protocol (formerly MessageFamily) type

• Testing:

  • Test deserialization/serialization of every message
  • Test deserialization from the old did:sov prefix.
  • Test deserialization/serialization of every message type
  • Test protocol registry minor version resolution
  • Test serialization equality between old messages and new messages (to be placed in the old messages crate)
  • Test ProtocolRegistry query lookup.
  • Test MaybeKnown serialization/deserialization to Known and Unknown.

• Renames:

  • Establish some naming convention for message types and messages type patterns (V1, V1_0, Kind, etc) - field containing types retained their name while message type enums now contain a Type in their name to avoid collisions.
  • Rename crate::msg_types::types module - renamed to crate::msg_types::protocols.
  • Rename crate::protocols::nameless module - renamed to crate::msg_fields::protocols.
  • Rename crate::message::Message to avoid confusion with AriesMessage - renamed to MsgParts.
  • Rename Nothing - renamed to NoDecorators.
  • Rename MajorVersion trait - renamed to ProtocolVersion.
  • Rename HasKind trait - renamed to MessageWithKind
  • Rename ConcreteMessage trait - renamed to MessageContent
  • Rename message kind enums, removing the Kind suffix. E.g: OutOfBandV1_1Kind to OutOfBandV1_1
  • Rename Actor to Role
  • Rename occurrences of MessageFamily or family to Protocol or protocol_name

This list is not exhaustive. Ideally, as progress is made or ideas are proposed, specific items would be added to it, such as:

• Rename This to Other
• Add tests for serialization/deserialization of Something

Any help in identifying, adding or even implementing items on this list is much appreciated. Reviewing the PR should result in ideas of what would be needed.

In terms of documentation, if it's not easily understandable in a short time (10-15 seconds) what a piece of code does and how, then that's a good sign that comments would be needed. I personally prefer exhaustive comments in favor of little/no comments.

Proposing items can be done by commenting on this PR or reaching out directly. I will then add them to the list. There's no bad idea of things that should be documented/renamed/tested/etc., so don't be shy.

[codecov-commenter]

Codecov Report

Merging #754 (9e64a10) into main (945a67a) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #754   +/-   ##
=======================================
  Coverage   54.66%   54.67%           
=======================================
  Files         381      381           
  Lines       36858    36864    +6     
  Branches     8123     8101   -22     
=======================================
+ Hits        20150    20154    +4     
- Misses      10706    10711    +5     
+ Partials     6002     5999    -3     

Flag	Coverage Δ
unittests-aries-vcx	54.56% <ø> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Patrik-Stas]

Barely went through everything but went through a little bit. Produced this small PR in the process
#755

[Patrik-Stas]

I'd suggest to take a look at some instance where breaking change protocol actually happened, like https://github.com/hyperledger/aries-rfcs/blob/main/features/0454-present-proof-v2/README.md
and try to add support for those messages. I think this would be good practical test as to how well the created protocol versioning abstractions, mechanisms hold.

[Patrik-Stas]

In PR #755 I added some tests, how would I go about it in the opposite direction on Message Family level. Eg something like this

#[test]
    fn connections_to_strings() {
        let msg_type = MessageType::new(Prefix::DidCommOrg, ConnectionV1_0::Invitation.into());
        assert_eq!(msg_type.to_string(), "https://didcomm.org/connections/1.0/invitation");
    }

Obviously I need to implement Display trait and this would be handy for logging in general. But I am not sure how go about implementation - surely we could reuse impl Serialize for MessageType to generate the final string for Display implementation?

[bobozaur]

I'd suggest to take a look at some instance where breaking change protocol actually happened, like https://github.com/hyperledger/aries-rfcs/blob/main/features/0454-present-proof-v2/README.md and try to add support for those messages. I think this would be good practical test as to how well the created protocol versioning abstractions, mechanisms hold.

To accommodate this in the future, we would expand the PresentProof message family and add a V2 variant and it's minor version variants.

Then create the "end messages" (e.g.: PresentProofV2_0), implement ConcreteMessage on them, create enum variants in the A2AMessage::PresentProof with V2 and minor versions as well and the rest will pretty much take care of itself.

[bobozaur]

In PR #755 I added some tests, how would I go about it in the opposite direction on Message Family level. Eg something like this

#[test]
    fn connections_to_strings() {
        let msg_type = MessageType::new(Prefix::DidCommOrg, ConnectionV1_0::Invitation.into());
        assert_eq!(msg_type.to_string(), "https://didcomm.org/connections/1.0/invitation");
    }

Obviously I need to implement Display trait and this would be handy for logging in general. But I am not sure how go about implementation - surely we could reuse impl Serialize for MessageType to generate the final string for Display implementation?

While using the Serialize impl would work, it's much faster to bypass the entire serde_json machinery and construct the message type directly.

I took the liberty of introducing MessageFamily::as_parts() which provide the constants declared within the types. The constants, along with the prefix, are then written to the string formatter in the Display impl for MessageType.

I also adjusted your tests to rather use MessageType::to_string() against &str types.

[bobozaur]

So, in the view of handling some common patterns that either already occur or should/will be occurring through the codebase, I gave some thought to how to approach messages in a more pragmatic way.
A similar problem was raised by @gmulhearn in #761.

The Problem

Primarily, messages and their decorators seem to be too intertwined which makes processing cumbersome, error-prone and not very contained.

If we think about threading, for example, it is pretty hard to come up with a way of replying to a message or starting a subthread in a consistent manner, irrespective of the message. Some messages need a Thread, others take an Option<Thread> and others don't have a thread field at all, so threading is always implicit.

Extracting the thread out of the data structure is one problem. There's also the need to extract the id sometimes, as if there's no thread, the id is used for implicit threading.

Proposed Solution

After some thinking and quite a few complex and ocult ideas, I came up with the following (reasonable, if you ask me) approach:

// NoDataNoProblem is a type which ignores any serialized (or absent) value and serializes to nothing. 
// Flattening nothing results in more nothing, so it's completely invisible when serialized.

#[derive(Serialize, Deserialize, Debug)]
pub struct Message<C, MD, FD = NoDataNoProblem>
{
    #[serde(rename = "@id")]
    id: String,
    #[serde(flatten)]
    content: C,
    #[serde(flatten)]
    msg_decorators: MD,
    #[serde(flatten)]
    field_decorators: FD,
}

What?

From a serialization/deserialization perspective, this structure has the same output/input as the current messages. The benefit is that by flattening the data into parts, we can destructure this type and do different things with it in different parts of the code.
E.g:

/// Current Request type
#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct Request {
    #[serde(rename = "@id")]
    pub id: String,
    pub label: String,
    pub connection: ConnectionData,
    #[serde(skip_serializing_if = "Option::is_none")]
    #[serde(rename = "~thread")]
    pub thread: Option<Thread>,
    #[serde(rename = "~timing")]
    #[serde(skip_serializing_if = "Option::is_none")]
    pub timing: Option<Timing>,
}

/// New Request type after monomorphization:
#[derive(Serialize, Deserialize, Debug)]
pub struct Message
{
    #[serde(rename = "@id")]
    id: String,
    #[serde(flatten)]
    content: RequestContent,
    #[serde(flatten)]
    msg_decorators: RequestMsgDecorators,
    #[serde(flatten)]
    field_decorators: NoDataNoProblem,
}

#[derive(Serialize, Deserialize, Debug)]
pub struct RequestMsgDecorators {
    #[serde(rename = "~thread")]
    pub thread: Option<Thread>,
    #[serde(rename = "~timing")]
    pub timing: Option<Timing>,
}

#[derive(Serialize, Deserialize, Debug)]
pub struct RequestContent {
    pub label: String,
    pub connection: ConnectionData,
}

It's probably clearer now that destructuring Message allows us to use RequestContent in the Connection protocol while handling the decorators (probably after first destructuring) can take place someplace else.

In the threading case, we can easily get our hands on the underlying Option<Thread> and even the id and use them to generate a new thread struct (as a reply, subthread, etc) while not interfering with what the protocol actually uses.

Also, there's nothing preventing the protocol state machines to accept some other things rather than just content. But I'm pretty sure most of the time it won't be required, so that actually ends up being more efficient since less data gets moved around unnecessarily through the stack frames.

FD (Field Decorators)

Not meaning to hate but I personally find this field decorator thing to be one of the worst designs in software development of this scale. I assume that the whole point of decorators is to have a set of fields independent of a protocol, and that's actually good.

I however do not see a reason why field decorators could not be transformed/incorporated into messsage decorators so you have a singular data structure serving a certain purpose rather than stuff spread all across the JSON ¯\(ツ)/¯.

In any case, when it comes to message processing, some field decorators are instrumental to the message and are practically part of it. These would not be defined here, but rather in the content.

However, stuff like some_field~l10n most likely doesn't have anything to do with the message processing itself. Not only that, it might be pretty useless if not handled along the ~l10n message decorator. So some_field~l10n would be a good candidate for the Field Decorators part of the message, so it can be easily extracted.

By default, the FD generic is set to (what I currently name) NoDataNoProblem which is a ZST that does absolutely nothing yet does not interfere with deserialization/serialization. So that means no field decorators are accepted (other than the ones declared within content).

So what's next?

I'll get to refactoring messages to use this pattern. There's still some experimentation I need/want to do before that, especially in terms of how the messages get segregated based on their @type. The approach can definitely accommodate that, but I just gotta settle on what would be a nice way to do it.

@Patrik-Stas, @mirgee We had the chance to talk a bit about this already, but still feel free to comment here any ideas/observations you might have. @gmulhearn, what do you think about this?

[bobozaur]

Combine MD and FD

Given the community call discussion, it seems like a good idea to actually combine the message decorators and field decorators into a structure defined for each message. The whole point is to destructure it anyway, so that's most likely fine.

Our goal is to mainly keep the protocol state machine relevant data separate from the protocol state machine agnostic data, so we can process them independently.

Field Decorators in nested JSON

Per the RFC, a field decorator is used as a sibling to the field it decorates. So they technically can be found at lower levels of the JSON.

I don't know of any actual implementations of something like that (apart from nested messages as with the OOB connectionless stuff, but that's handled as a contained message).

If, however, something like that surfaces, I think we can easily accommodate it to the same message structure that we have, with the caveat of doing some conversions through some intermediary types to normalize the data.

[bobozaur]

Finally, the messages2 crate is feature complete. The initial comment will be updated with a list of completed/remaining tasks.

[Patrik-Stas]

Good job! The

Message<Content, Decorator>

and the related traits, the MessageWithKind blanket impl etc. seem to play pretty well together.

Just please remove reformatting changes applied across the board in unrelated crates. (it also makes github really slow, making it bit more difficult to review, as there's many modified files)

Some of the unfinished pieces which are not too critical good be very good first issues for newcomers! Leave them something! :-) This is good and well organized code to learn from. I left suggestions to do this with some of my review comments.

[Patrik-Stas]

Great job, left a few minors comments but green light from me 🟢

[mirgee]

Just minor nitpicks otherwise LGTM 👍