Refactor/typestate connections

[bobozaur]

Very basic draft implementation for a typestate Connection type.

Note that the inner methods modifications are minimal for now, as the first step was to transpose the nested enums to typestate.

[Patrik-Stas]

My thoughts:

• The new implementation is nice in terms of structuring. In yesterday private discussion we it was suggested that state type pattern would remove the need for the "handlers" layer but it seems to be that Connection<I, T, S> would become equivalent of the current handlers layer, eg the currentimpl Connection { (src/handlers/connection/connection.rs).

• My main problem with the new implementation is following:

  • With existing implementation, the presence of states is hidden internally behind impl Connection handler/facade. Altough this enables consumers of Connection to call wrong methods (and be punished with error :-)), it on the other hand makes the client code simpler - and especially in terms of de/serialization. Lets' take following example:

  let connection_serialized = database.load_connection_by_id("alice");
  let connection = Connection::from_string(connection_serialized);
  connection.some_operation()?; // we might get error if `some_operation` did not 
                                // make sense in whatever state `connection` was at
  

  However, with the new approach, assuming Connection<I, T, S> would present the public API for aries-vcx consumers, it gets trickier, but let me try anyway: (note I just ignore the transport / T generic arg)

  let connection_serialized = database.load_connection_by_id("alice");
  let connection: Connection <Inviter, T, CompletedState> = Connection::from_string(connection_serialized);
  connection.some_operation(); // this is safer to call now, but we had to know the state ahead of time
  

  well that works, but the developer needs to already know if the serialized connection was inviter or invitee and what state is that connection at, as it's now required at compile time. Which seems kinda impractical.

  Perhaps the state pattern can be still adopted internally, but impl Connection<I, T, S> { } would be internal model, and this would be wrapped by "handler" like the impl Connection in /handlers today.

• More of a side note: I like that this PR is addition of alternative implementation instead of mutating and disrupting existing code. The new Connection<I, T, S> structure would serialize obviously different and though possible to retrofit on old serialization format, I am not even sure if that would be worthy the effort. If this will move forward, I'd suggest continue in this fashion (perhaps even more structurally separate old/new approach), deprecate old style, let people migrate, delete old code.

Drafted myself a small diagram while reviewing this, so I'll just drop it here

[bobozaur]

@Patrik-Stas That's a very good observation and yes, that's the disadvantage described by me as well.
However, the serialization/deserialization scenario can still be worked around by introducing helper structs and perhaps even a trait for making things easier for users.

An oversimplified example is:

// User code:
enum MyStoredConnection {
    Http(StorableConnection<HttpClient>),
    Websocket(StorableConnection<Websocket>)
}

// Library code:
pub enum StorableConnection<T> {
    Invitee(StorableInviteeConnection<T>),
    Inviter(StorableInviterConnection<T>)
}

pub enum StorableInviteeConnection<T> {
    Initial(CommonConnectionData<T>),
    Invited(CommonConnectionData<T>),
    Requested(CommonConnectionData<T>),
    Responded(CommonConnectionData<T>),
    Complete(CommonConnectionData<T>)
}

pub enum StorableInviterConnection<T> {
    Initial(CommonConnectionData<T>),
    Invited(CommonConnectionData<T>),
    Requested(CommonConnectionData<T>),
    Responded(CommonConnectionData<T>),
    Complete(CommonConnectionData<T>)
}

pub struct CommonConnectionData<T> {
    marker: PhantomData<T>
}

pub trait StoredConnections {
    type StorableConnection;

    fn new() -> Self;

    fn store_inviter_initial<T>(&mut self, con: Connection<Inviter, T, InitialState>) {}

    fn store_inviter_invited<T>(&mut self, con: Connection<Inviter, T, InvitedState>) {}

    fn store_inviter_requested<T>(&mut self, con: Connection<Inviter, T, RequestedState>) {}

    fn store_inviter_responded<T>(&mut self, con: Connection<Inviter, T, RespondedState>) {}

    fn store_inviter_complete<T>(&mut self, con: Connection<Inviter, T, CompleteState>) {}

    fn store_invitee_initial<T>(&mut self, con: Connection<Invitee, T, InitialState>) {}

    fn store_invitee_invited<T>(&mut self, con: Connection<Invitee, T, InvitedState>) {}

    fn store_invitee_requested<T>(&mut self, con: Connection<Invitee, T, RequestedState>) {}

    fn store_invitee_responded<T>(&mut self, con: Connection<Invitee, T, RespondedState>) {}

    fn store_invitee_complete<T>(&mut self, con: Connection<Invitee, T, CompleteState>) {}

    fn store_inviter<T>(&mut self, con: StorableInviterConnection<T>) {
        match con {
            Initial(con_data) => self.store_inviter_initial(con_data.into()),
            Invited(con_data) => self.store_inviter_invited(con_data.into()),
            Requested(con_data) => self.store_inviter_requested(con_data.into()),
            Responded(con_data) => self.store_inviter_responded(con_data.into()),
            Complete(con_data) => self.store_inviter_complete(con_data.into()),
        }
    }

    fn store_invitee<T>(&mut self, con: StorableInviteeConnection<T>) {
        match con {
            Initial(con_data) => self.store_invitee_initial(con_data.into()),
            Invited(con_data) => self.store_invitee_invited(con_data.into()),
            Requested(con_data) => self.store_invitee_requested(con_data.into()),
            Responded(con_data) => self.store_invitee_responded(con_data.into()),
            Complete(con_data) => self.store_invitee_complete(con_data.into()),
        }
    }

    fn store_connection<T>(&mut self, con: StorableConnection<T>) {
        match con {
            Invitee(invitee_con) => self.store_invitee(invitee_con),
            Inviter(inviter_con) => self.store_inviter(inviter_con)
        }
    }

    fn handle_storable_connection(&mut self, con: Self::StorableConnection);
}

The idea is that the Connection<I, T, S> would serialize to StorableConnection (can be done under the hood by serde) and users would just wrap it to provide transport distinction (not mandatory if only one transport is used throughout the agent - StorableConnection<Transport> can be used in this case). Deserialization would happen to the user defined type (or StorableConnection<Transport> and could be uniformly handled through a StoredConnection trait impl.

An implementor of StoredConnection can decide which connections it wants to store (as all default to no-op) and how to store them and use them throughout the rest of the code (could use some type-based HashMap, Box<dyn Any>, multiple HashMap or Vec, channels for sending the connections to other parts of the code, etc.). The only "mandatory" method to implement would be handle_storable_connection where users implement the transport distinguishing logic.

NOTE: An idea might be to return errors from the trait methods, but this was merely written as an example.

This is indeed more advanced but definitely not extreme.

[Patrik-Stas]

I like the further separation of old/new code! As you know, in the next step I'd mostly like to see usage of this in libvcx. I'd suggest there to keep similar separation between the old/new approach so we don't break libvcx wrappers.

[codecov-commenter]

Codecov Report

Merging #739 (3cb83fb) into main (66ee076) will decrease coverage by 24.77%.
The diff coverage is 49.22%.

@@             Coverage Diff             @@
##             main     #739       +/-   ##
===========================================
- Coverage   54.73%   29.96%   -24.77%     
===========================================
  Files         363      379       +16     
  Lines       37122    30910     -6212     
  Branches     8089     6283     -1806     
===========================================
- Hits        20317     9262    -11055     
- Misses      10832    18939     +8107     
+ Partials     5973     2709     -3264     

Flag	Coverage Δ
unittests-aries-vcx	29.96% <49.22%> (-24.67%)	⬇️

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

Impacted Files	Coverage Δ
aries_vcx/src/handlers/connection/cloud_agent.rs	32.20% <ø> (-42.21%)	⬇️
...vcx/src/handlers/connection/mediated_connection.rs	50.07% <0.00%> (-13.00%)	⬇️
aries_vcx/src/handlers/mod.rs	0.00% <ø> (-8.58%)	⬇️
aries_vcx/src/handlers/out_of_band/receiver.rs	0.00% <0.00%> (-39.05%)	⬇️
aries_vcx/src/lib.rs	100.00% <ø> (ø)
...vcx/src/protocols/connection/generic/thin_state.rs	0.00% <0.00%> (ø)
aries_vcx/src/protocols/connection/trait_bounds.rs	0.00% <0.00%> (ø)
...ocols/mediated_connection/inviter/state_machine.rs	69.06% <ø> (ø)
aries_vcx/src/protocols/trustping/mod.rs	75.00% <0.00%> (-10.72%)	⬇️
aries_vcx/tests/test_connection.rs	100.00% <ø> (+25.35%)	⬆️
... and 333 more

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

[Patrik-Stas]

Hi @gmulhearn have a look at this, I am interested to hear your thought on this. Consider waiting until today's video recording is uploaded on https://wiki.hyperledger.org/display/ARIES/2023-01-26+Aries-vcx+community+call where @bobozaur goes through the approach taken here.

[bobozaur]

Necessary changes from #741 to get the wrapper working are done. I'd say this PR is in it's final state, so please feel free to review it.

[gmulhearn]

Just finished a pass thru and couldn't really find anything other than a few small things. Looks really good. I'm not too versed with the napi stuff, so I have no comments there.

[gmulhearn-anonyome]

One oddity I've noticed is that the invitee's request message it makes the message's thid equal to the invitation message ID, rather than a new unique ID. Is this ok? IMO it would be cleaner and potentially less confusing if the invitee sets the thid of this protocol to a new unique ID. Or have I misunderstood something?

Also another thing;
it would be nice if the Request message's ID is also set as a UUID rather than using a default MessageId of ("testid").

[bobozaur]

@gmulhearn I replicated the old behavior, so I'm not 100% certain of the implementation details, but from my limited understanding a pairwise invitation is for a pair of agents. You wouldn't expect a second invitee to accept it. So the thread ID can remain the invitation ID, as it leads to contiguous context tracking.

For the Request ID, it's out of scope for this PR. It's the old behavior and while I agree it should change, probably many things could/should 😅 .

[Patrik-Stas]

The current failure on nodejs serialization in CI is fixed in this PR

#749

[Patrik-Stas]

@gmulhearn is testid actually ever used as value in non-test context?
@gmulhearn About the logic of setting @id as response to pairwise invitation, I agree with @bobozaur in scope of this PR I'd just stick to original behaviour, for starter.
We can discuss this separately, but I'll briefly share my opinion - I think this is actually right behaviour, as in case of connection protocol kicked-off by inviter sending pairwise invitation, that's esentially the first agent2agent (altough out of band) message kicking off the protocol with certain @id. Then the connection-request just continues in that thread (dictated by invite @id).

[Patrik-Stas]

Let's wait for @mirgee @gmulhearn approvals and then we can merge

[gmulhearn]

Lgtm!

[gmulhearn]

Kk no worries, I might open an issue where further discussion can happen. But ye agreed, not in scope here