status-im / status-protocol-go

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

Experiment with Message #50

Closed adambabik closed 4 years ago

adambabik commented 5 years ago

This is a collection of a few experiments. It's not intended to be merged unless everyone support this. The main goal is to propose a few solutions which are interesting and could be worked on in a separate PR.

Experiments

new-migration Makefile target

To simplify and unify the process of creating migrations.

Removal of adapters.go

In my opinion, the way we currently implemented individual layers makes no room for adapters.go and it can be removed.

messageProcessor

Introducing messageProcessor which allows to process a message through various stages depending whether a message was received from the network (Resolve) or created and pushed to the network (Send). It encapsulate the flow and error handling for encrypting, data syncing and encoding.

Message --> MessageLegacy

The Message struct required to move messages persistency from status-react was renamed to MessageLegacy.

New Message

New Message struct in the root directory of the project encapsulates protocol messages and exposes methods to encode, encrypt, wrap etc. It also has many utility functions. Generally, the goal was to store all Message-related method in one place.

adambabik commented 5 years ago

@cammellos @PombeirP do you consider anything from this PR being particularly interesting and worth taking out into a separate PR in order to merge it?

cammellos commented 5 years ago

I think migrations are ok

remove adapters looks also ok

message/ message processor roughly looks good, minor a few things, seems like Message knows about protocol.Message/protocol.Whatever, I would strive to avoid that (if we use interface{} as we do), so isUserMessage() should not check for it being a protocol.Message, similarly Clock etc

legacy I am not sure, currently we have a bunch of deprecated methods but not clear exactly what the replacement is and why they are deprecated, as we don't provide any alternative or the alternative are not really fulfilling the necessary scenarios to implement a fully fledged client, so to me is just confusing and it makes it unclear what actually we provide an alternative to or more works need to be done.

To deprecated things/mark them as legacy the flow in my opinion should be:

1) Write an equivalent replacement, make sure it works with a fully fledged client (i.e. Join does not work with group chats, Leave completely deletes the chat which is not the desired behavior nor handles group chats, no way to retrieve messages from mailservers without using LoadFilters etc) 2) Mark deprecated the method, explaining what the replacement is and how to use it 3) Move clients

The question should always be, if anyone wants to build a full client today, can they use a different method? if yes, then it's fine to mark as legacy/deprecated (unless we decide to completely stop providing that feature, which requires a bigger discussion), otherwise that's really the best we got, as much as we don't like it, plus is clearer what we still need to work on, and what is completed

adambabik commented 5 years ago

I would strive to avoid that (if we use interface{} as we do), so isUserMessage() should not check for it being a protocol.Message, similarly Clock etc

That's a good point. Actually, this is used exclusively in persistence.go and we only want to store user messages so value of interface{} + public key should be enough and we can get rid of these methods.

I agree regarding legacy/deprecated stuff. My thinking when we started porting stuff from status-react to status-protocol-go was that many things would be marked as legacy/deprecated and equivalents would be provided and used in the console client to implement the same feature. It turned out that there are more unknowns and time-pressure to do that effectively. But I think we are coming to the point where we can get back to this idea.

Of course, we can wait with marking anything legacy/deprecated. I guess the best approach would be to make a list of tasks for the console client to be on par with the existing clients. Based on these tasks we can start with implementation/fixes and identify potential deprecations and provide equivalents.

cammellos commented 5 years ago

@adambabik yes fully agree

I guess the best approach would be to make a list of tasks for the console client to be on par with the existing clients.

sounds like a plan!

adambabik commented 4 years ago

We did everything we wanted from this PR in separate PRs.