search

status-im / status-protocol-go

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

Add datasync #34

Closed cammellos closed 5 years ago

cammellos commented 5 years ago

This commit adds the ability of reading and sending datasync messages. Those messages are PFS encrypted, so the structure of the final messages is:

Whisper
--------------
Encryption
--------------
Datasync
---------------
Message 1 Message 2 Message 3
----------------

Where Message is either a protobuf message wrapping transit, or just transit for pre-v1 compatibility.

Datasync messages are batched, so a single whisper message might contain multiple protocol messages. Datasync messages are parsed regardless of whether the datasync flag is on, as this commit is backward compatible.

Sending messages is toggled off by default.

Tested against console client, draft as mvds PR is still pending https://github.com/vacp2p/mvds/pull/61, but feedback is welcome.

oskarth commented 5 years ago 
Whisper
--------------
Encryption
--------------
Datasync
---------------
Message 1 Message 2 Message 3
----------------

Is there a corresponding PR in https://github.com/status-im/specs repo for this?

cammellos commented 5 years ago

@oskarth no, I think though that we should probably keep docs in this repo. First it's easier to keep things in sync, and also the toll currently on making a protocol change can be as high as 6 PRs:

1) Update mvds 2) Update status-protocol-go 3) Update status-go 4) Update status-console-client 5) Update status-react 6) Update specs

So one less PR (specs) would make this less cumbersome (we can still track those file in that repo as a submodule or whatever, as long as updating code & specs is atomical)

oskarth commented 5 years ago

Hm, my concern is mainly that the specs repo should be self-contained and allow implementation by another client. If it's just clarification purposes / implementation details then having it as docs in relevant repo seems fine.

I'm not sure why it'd require extra PRs to status-react (outside of normal ones). Having some friction for protocol changes is also not necessarily a bad thing!

@decanus what are your thoughts?

cammellos commented 5 years ago

I think we should though take into consideration our workflows.

Who makes changes to the protocol? Status through this repo. If at some point that changes, and this repo is not the reference implementation anymore, then we should change it, but for now we would be decoupling two things that are completely coupled.

For visibility, we can still have a repo tracking the docs files, but having them decoupled in our current scenario is providing no benefits and increasing the likelihood of them not being in sync.

In terms of friction, there's definitely enough, any change here, needs to be somehow validated by at least one client, and currently that's really status-react as it's the only fully implemented client (ideally it will be status-console-client at some point), which includes a change in status-go, so that's 5 PR just to make sure the changes are ok. (not validating the changes with a client is also possible, but I don't think it's advisable)

cammellos commented 5 years ago

@adambabik ready for review again @oskarth probably is best if either you or @decanus updates the specs repo, as you are more familiar with the inner working of datasync and we can probably leverage existing docs. Thanks!

cammellos commented 5 years ago

@adambabik addressed feedback, thanks!

What's the relation between this flag and sendV1Messages? Do we want to support V1 messages with or without ds?

For now I think it's useful to support both, so we can test the two independently or together (it makes it easier understand where is the bug), and it comes at little complexity cost, before the v1 final cut, we can remove the code as not necessary anymore.