status-im / status-protocol-go

Status Protocol implementation in Go
Mozilla Public License 2.0
0 stars 1 forks source link

Provide a way to retrieve messages from all active filters #24

Closed adambabik closed 5 years ago

adambabik commented 5 years ago

As a user of the library I want to be able to retrieve all messages from all active filters so that I can see messages from chats I haven't created yet.

Description

Currently, Messenger provides Retrieve() method but it requires passing a Chat instance.

As we know that on the same Whisper topic there might be many chats, also ones which have been started by another user, we want a way to retrieve all messages for all chats, even not created yet.

This ticket is required for #2, however, #2 has even more requirements like retrieving all types of messages (not only protocol.Message) and retrieving raw transit encoded payloads.

This method would be useful for improving status-console-client experience as it does not display chats started by other peers.

Acceptance criteria

Messenger.RetrieveAll() (messages []*protocol.Message, err error) is provided.

cammellos commented 5 years ago

RetrieveAll() should also return the chatID of the chat we used to install the filter, that's necessary for public chats to work (currently that's what we use to know a public message is for a given chat).

adambabik commented 5 years ago

I see two options here:

adambabik commented 5 years ago

Also, please read the description:

This ticket is required for #2, however, #2 has even more requirements like retrieving all types of messages (not only protocol.Message) and retrieving raw transit encoded payloads.

This method would be useful for improving status-console-client experience as it does not display chats started by other peers.

And acceptance criteria:

Messenger.RetrieveAll() (messages []*protocol.Message, err error) is provided.

To create another set of methods to support status-go, there will be a different ticket, if needed, or it will be a part of https://github.com/status-im/status-protocol-go/pull/27 PR.

Sorry for the confusion, I created this ticket to rather be like a warm-up one :) Not necessarily digging into the nitty-gritty of status-go integration.

cammellos commented 5 years ago

Rely on the filed in protocol.Message.Content.ChatID

Currently not all messages have a chatID, although it s really only relevant to text messages. So this might be non encompassing solution.

Add protocol.Message.ChatID

I would do this, and as well add a flag Public Private to know whether is coming from a private or public topic, although this decision can be always computed by checking back against the loaded chats, but might simplify the checking

adambabik commented 5 years ago

@PombeirP and I had a call about this ChatID thingy. This is a summary of what we discussed.

We can think about ChatID as a session-long identifier of each chat. A session lasts as long as the Whisper filters exist which in our case translates to as long as the Status app stays in the memory.

We can assume that ChatID is something created by the protocol lib to uniquely identify joined chats. This imply that each chat, before sending a message to it or retrieving messages from it, needs to be joined first. This can be done through initial public chat names and public keys passed to Messenger constructor or using Join() method.

Returned ChatIDs should be stored by the caller. Each ChatID will correspond to a public chat name or public key and it should be callers responsibility to store this mapping, however, as mentioned at the top, it's only per session so no persistent storage is required.

When sending a message, only ChatID should be sent by the caller. If a given ChatID is not found, an error will be returned, otherwise, ChatID will uniquely identify details of a joined chat and will send a message. Similarly requesting for a single chat messages will work.

The difference will be in a case of requesting all messages. In this case, messages will be returned with corresponding ChatIDs.

cammellos commented 5 years ago

sure, however we want to rationalize it.

We should mind though that what should drive our decisions is the only fully fledged client we have, namely status-react, and the reasoning should start from there, otherwise we end up as we did before going on a tangent and writing code that does not support our only production ready client.

For example:

This imply that each chat, before sending a message to it or retrieving messages from it, needs to be joined first.

That's not how our current client works, you don't have to join a one to one or group chat to receive messages from it (we have a join system in group chat, but it's different semantic). And we should not change this behavior unless necessary, at least for v1, in order to minimize the changes, post v1, we can re-discuss and spent more time if needed.

There are other nuances that it's clear from this discussions that you are not aware of (We send messages to users without having "Joined" a chat, but loaded only loaded a filter for those, such in contact updates, group members etc), so not always we want to create a corresponding chat (or we want to create one).

That's just to stress that our decisions and understanding should be driven not from this project and should not be done in a bubble. This is only an incomplete view of all the uses cases, as a lot of logic is still in status-react, which still will need to be moved here, so understanding those flows is crucial before writing a cohesive api.

adambabik commented 5 years ago

That's not how our current client works, you don't have to join a one to one or group chat to receive messages from it (we have a join system in group chat, but it's different semantic).

Right but you need to create a filter first and from that filter, there might be many chats originating. My description is definitely incomplete because I tried to only depend on ChatID term which is not enough.

Generally, I think we need to rationalize some aspects now because otherwise it still gonna be confusing as hell just in Go not in Clojure :) But of course to some extent, as you said, thinking about the current consumers of this lib.

It seems like this topic is VERY confusing so let's try to rationalize it just a bit.

So far, we had this filter.Chat as an input and output. Let's at least decouple that. Currently, in this lib there is a proposal of this interface:

type Chat interface {
    ID() string
    PublicName() string
    PublicKey() *ecdsa.PublicKey
}

as input for joining/sending/retrieving messages from a particular chat. The question is, is this enough (skip group chats for now) but extensible so that it can support group chats as well? Is it proper to assume that ID is given by the caller or it should be generated and returned by this lib?

Next, what should be the output? Do we need to send the whole filter.Chat along with messages or only specific information from filter.Chat is needed? Or maybe filter.Chat is created based on the current implementation and we want to keep it this way for now?

pedropombeiro commented 5 years ago

While I think that we definitely need to spend some time thinking about how we want the chat ID to work (ownership, lifetime, etc.), I think it's best done in a separate PR, as that would represent enough complexity by itself. Yesterday after discussing with Adam I implemented something very similar to what was suggested in the latest comments (minus Chat interface changes), and I'd appreciate your feedback.

cammellos commented 5 years ago

I agree, maybe let's park the conversation for now, unless really necessary to move forward.

Next week we can go through the mechanics of the various types of chats and figure out a reasonable abstraction/rationale so we are all on the same page.

what do you think?

pedropombeiro commented 5 years ago

I think we can move forward with this PR without those changes, even though perhaps the data structures won't be as tight as they could be. Sounds good.

adambabik commented 5 years ago

I created https://github.com/status-im/status-protocol-go/issues/28 for further discussion on this topic.