First cut of Tapir Integration

Review Request #10 — Created July 22, 2019 and updated

Updating test scripts to remove peer

First cut of automatic acknowledgements and protocol contexts

Fixing Rebase Artifacts

  • 0
  • 0
  • 11
  • 0
  • 11
Description From Last Updated
  1. Ok this is my first pass review too :) :

    The connection manager has AttemptReconnections which is now lost, so we have no structure arround attempting reconnections if the first one fails

    additionally, as has been pointed out in the PR lower, there is no handler for failed connections
    So it appears that the ui wil be sad, as no "CONNECTING" event is fire, and also no "FAILED". We need to capture thsi struture somewhere

    1. Because we have the event bus now, the UI can be in charge of deciding if it wants to attempt reconnections (by listening too Connection State changes and enacting a strategy) Because reconnecting is expensive we will likely want to define different strategies and allow users to pick between them e.g. "mobile optimized / always retry / retry every X minutes etc."

  2. protocol/connections/engine.go (Diff revision 1)

    I love show barebones this Interface is now. Kinda how we want these freefloating event drive things to be. Tho it does mean we may want to invest in docs more cus the clarity of what it does and handles is a bit more obscured... ¯_(ツ)_/¯

  3. protocol/connections/engine.go (Diff revision 1)

    message types like "im.cwtch.invit" should be consts exported from somewhere

  4. protocol/connections/engine.go (Diff revision 1)

    How do you mean? remove this pass through when ui does what?

    1. There should probably always be a context, in which case we should drop messages without one. But I want to integrate the UI before asserting that behaviour completely.

  5. protocol/connections/engine.go (Diff revision 1)

    make a const

  6. protocol/connections/engine.go (Diff revision 1)

    need a

    e.eventManager.Publish(event.NewEvent(event.PeerStateChange, map[event.Field]string{
            event.RemotePeer:      string(onion),
            event.ConnectionState: ConnectionStateName[CONNECTING],

    to alert the UI we are attempting to connect
    right now we only appear to alert the ui on AUTHENTICATION and DISCONNECTED

  7. protocol/connections/engine.go (Diff revision 1)

    what is eventId in ricochet protocol speak?

    so this is just surfacing to the even system tapir acked a message we received?

    1. Yeah, this basically just bridges the gap between the event bus events and the tapir events.

  8. protocol/connections/engine.go (Diff revision 1)

    does this get called when a connection fails? or do we have/need a peerFailed

    cus the current flow attempts connections and emits a PeerStateChange -> FAILED when connections fail, which is fine, going to DISCONNECTED is the same, i see no problem with that

    1. I think collapsing it down to DISCONNECTED is fine, we should catch all potential failure cases (e.g. not an onion address etc.) before, so the only valid interpretation should be "this peer is not online / can't be reached"

  9. protocol/connections/engine.go (Diff revision 1)

    This seems a failure of the Interface

    tapir/peerapp seem to build a cwtch peer that can respond to messages quietly on it's own and emit events for various incoming message behaviour but has no API for publishing messages thus necesitating this akward force cast. That seems like a sign the API somewhere needs the concept of sending at least at some abstract level or handler

    1. This is by design. Tapir Apps have a very limited interface (and so must be type asserted when interacting with) - otherwise we are defining an interface for Tapir Apps which I want to avoid (A Tapir App for a CwtchServer will look very different to a Tapir App for a Cwtch Peer - the apps interact with a well defined interface of connections, but we leave the App interface mostly undefined)

  10. protocol/connections/engine.go (Diff revision 1)

    i hate panics in such a multi threaded system that backs an ui/app.

    log.Error and send an error event to the event bus, or leave blank if it truely should never happen?

    I guess it's a style thing

  11. protocol/connections/engine.go (Diff revision 1)

    shouldn't this be

    if cpp.GetGroupChatInvite() != nil {
      handle and publish NewGroupInvite
    } else {
      publish NewMessageFrom Peer

    and not

    if no unmarshal err {
      if group invite {
         handle and publish group invite
    publish new message

    looks like it'll dup for group invite and publish message even if there was an error?

    1. Yup, good catch.

  12. protocol/connections/peerapp.go (Diff revision 1)

    prolly should be a const

    prolly all of them can be here in PeerApp?

Review request changed






Revision 2 (+1821 -1153)

Show changes