`NetworkSubsystem` Review about automerge-repo HOT 2 CLOSED

Totally agree, and in fact during the last couple of days I was working on exactly this, sorting out the message types and replacing channelIds with fields that actually name what they contain.

Reposting FWIW this conversation from Discord from a few days ago:

@HerbCaudill: In the auth work that I'm slowly picking back up from a couple of months ago, I'd gone partway down the road of creating a new GrossChannelId™ situation like the one for ephemeral messages, but for authentication messages (instead of the channel ID being m/${documentId} it would be something like a/${shareId}).

I'd actually love to straighten out that whole mess instead.

As far as I can tell, there are currently just two types of messages: Automerge sync messages, and ephemeral messages used by the application for presence etc. So I'm thinking of putting together a PR that refactors message types, along these lines:

type SyncMessage = {
  type: 'SYNC_MESSAGE'
  senderId: PeerId
  recipientId: PeerId
  documentId: DocumentId
  payload: Uint8Array // Automerge binary sync message
}

type EphemeralMessage = {
  type: 'EPHEMERAL_MESSAGE'
  senderId: PeerId
  documentId: DocumentId
  payload: unknown // up to the application
}

// eventually

type AuthMessage = {
  type: 'AUTH_MESSAGE'
  senderId: PeerId
  recipientId: PeerId
  shareId: ShareId
  payload: unknown // up to the application
}

type Message = SyncMessage | EphemeralMessage | AuthMessage

Does that seem like a reasonable approach? Am I correct in saying that currently those are the only two message types? (Outside of the ones used internally by the various network adapters, like arrive/welcome/join)

FWIW I've tried to inventory current usage here: https://gist.github.com/HerbCaudill/8f6b36acf20ed0477b2dda2d86a6793c#file-automerge-repo-message-types-md

(questions are primarily for @pvh who knows where the bodies are buried but would welcome anyone's input)

@pvh: i think that's roughly correct
i think making sync messages explicit is a very good idea
one thing to think about is what a channel is / should be
your earlier implementation had sort of a channel-per-document, so in that vein you'd want to be able to send sync, ephemeral, and auth messages around a topic on one channel and then route them based on type instead
why didn't i do that, i wonder

@HerbCaudill: Auth messages don't pertain to a document
I don't think a channel is a thing at all
Like, we're only using channel IDs to stand in for other things
I was also wondering about the broadcast bit - as far as I can tell it's only true for ephemeral messages

@pvh: That's probably the right answer
It's only used for ephemeral messages to get them forwarded on, it's true. It's also badly implemented, in that there's not currently any guard to make sure that cycles don't emerge.
BUT
the broadcast bit or a "*" destination is more generically useful; the sync protocol does not but should allow us to create a broadcast message for locally generated messages
(assuming you're in sync with another peer, you don't need to do any work to know that you should send them the one locally originating message)
(nor does any other peer have to check if they need it, or whatever -- just blast it out there to everyone)

@HerbCaudill: I'm not following. Why would you want to broadcast a sync message?

@pvh: oh, you could send a change with no requests to everyone
right now you have to individually call generateSyncMessage for each peer
i guess to really get this right the function signature would optionally include a list of peers whose syncStates you'd update locally so you don't double-send them the change

@HerbCaudill: got it

I'm currently trying out a slightly different type structure for messages, with a consistent outer layer containing message type & to/from information, and a payload object that varies by message type — kind of like a Redux action:

type MessageEnvelope = {
  senderId: PeerId
  recipientId: PeerId
}

export type SyncMessage = MessageEnvelope & {
  type: "SYNC_MESSAGE"
  payload: {
    documentId: DocumentId
    automergeSyncMessage: Uint8Array
  }
}

export type EphemeralMessage = MessageEnvelope & {
  type: "EPHEMERAL_MESSAGE"
  payload: {
    documentId: DocumentId
    encodedMessage: Uint8Array // CBOR-encoded message created by application
  }
}

// eventually

export type AuthMessage = MessageEnvelope & {
  type: 'AUTH_MESSAGE'
  payload: {
    shareId: ShareId,
    // ...
  }
}
export type Message = SyncMessage | EphemeralMessage | AuthMessage

Regarding the broadcast bit, I take Peter's point that we might want to broadcast some sync messages in the future. In the meantime, since ephemeral messages are the only ones that are broadcast, I think it makes sense to leave out the broadcast property and just go by message type.

from automerge-repo.

The issue with that is what the "recipientId" is, then. I tried setting it to a faux "*" recipient for a while but that led to problems with deciding who to rebroadcast to. I think it's important to think a bit about what the promises of the network layer need to be: what kinds of guarantees do we assume? In-order delivery?

from automerge-repo.