Initial implementation #4

oxtoacart · 2021-03-26T13:54:20Z

Do the tests pass? Consistently?
Did this change improve test coverage?
Has it been reviewed/approved by relevant teammates?
Can it be QA’d in staging or something like staging?
Is the code in question being linted? If not, consider adding a linter step to CI. If yes, make sure the linter is happy.
Do we know how we’re going to deploy this?
Are we clear on what the support and maintenance impact is?
Did you create new documentation? Is it accessible and in the right place?

…e key

…ppy path test

…n database

… than previously recorded mostRecentMessageTime

atavism · 2021-05-11T20:51:34Z

README.md

@@ -0,0 +1,62 @@
+# messaging-android
+
+## Data Model


Could we possibly add a brief overview to the top of the README instead of starting out with the data model? That this is an Android messaging library, Lantern Android integrates it, it re-uses code from Signal's protocol, etc?

Done. Let me know if there's anything else you'd like to see in there.

atavism · 2021-05-11T21:01:00Z

@oxtoacart Just to clarify, is there a PR yet on https://github.com/getlantern/android-lantern with only your messaging changes or are all those on the ox/flutterport branch already?

oxtoacart · 2021-05-11T21:19:51Z

@oxtoacart Just to clarify, is there a PR yet on https://github.com/getlantern/android-lantern with only your messaging changes or are all those on the ox/flutterport branch already?

No, there's no PR yet, we're working on a very long-lived ox/messaging branch.

messaging/src/main/java/io/lantern/messaging/metadata/Metadata.kt

atavism · 2021-05-17T14:19:59Z

messaging/src/main/java/io/lantern/messaging/Messaging.kt

+            Metadata.analyze(file, mimeType)
+        } catch (t: Throwable) {
+            logger.error("couldn't analyze metadata: ${t.message}")
+            null


Do we present any sort of error to the user if creating/storing an attachment fails?

if creating the attachment fails (like if there's a problem writing to disk), we'll throw an exception that can be handled by the UI. This particular branch just means that we couldn't extract metadata for the attachment, which doesn't prevent us from creating the attachment, it just means we don't have any metadata. That particular case is not indicated to the user, which I think is okay since there's nothing they could do about it anyway.

…lly deletes message

hwh33

Awesome work @oxtoacart, this looks great! I've looked at everything but the tests so far. I left a number of comments, but I think a lot of them are just clarifying questions. I ended up looking at db-android a bit - cool stuff! Nice all around really

hwh33 · 2021-05-18T02:28:50Z

README.md

+## Protocol Buffers
+messaging-android communicates with Tassis and internally stores data using protocol buffers.
+Messages exchanged with tassis are defined in [Messages.proto](messaging/src/main/protos/Messages.proto),
+which is just a copy of the protocol buffers defined in [tassis](https://github.com/getlantern/tassis/blob/main/model/Messages.proto).


There must be a better way of sharing this model. Perhaps a repo which defines the shared models, even if it's currently only this file?

I'm on the fence about this. I agree it's a little iffy to copy it, but OTOH a git submodule for just one file seems like a lot of work of its own.

As a compromise of sorts: what if you added a note at the top of this file like:

Copy of getlantern/tassis/model/Messages.proto Version: 39ca42e If you update this file, please update this note as well.

messaging/src/main/protos/Messages.proto

hwh33 · 2021-05-22T17:19:51Z

messaging/src/main/protos/Messages.proto

+
+package tassis;
+
+option go_package = "github.com/getlantern/tassis/model";


model is not a great name for a Go package. Will there be no other users of this model? In that case, maybe it could be an internal package? If there will be external users, then maybe it could just go directly in getlantern/tassis? Or maybe getlantern/tassis/tassismsg?

What's wrong with model?

It's generic; it doesn't convey any meaning about the contents. This may not be the model for importers outside of the tassis world. Other importers may have their own models for storing messages and other constructs defined in model.
As an analogy, it's like having a protocol library with subpackages client and server. Importers can always alias it as tassismodel or whatever they like, but then everyone is referring to your package differently.

If you're particularly tied to model, I won't fight you on it, but I do think it makes for awkward usage.

Ah, now I understand. Let me think about that.

messaging/src/main/protos/Messages.proto

messaging/src/main/java/io/lantern/messaging/CryptoWorker.kt

messaging/src/main/java/io/lantern/messaging/Messaging.kt

messaging/src/main/java/io/lantern/messaging/CryptoWorker.kt

messaging/src/main/java/io/lantern/messaging/Messaging.kt

oxtoacart · 2021-06-08T12:37:25Z

Thanks for the comments @hwh33! I've responded to all and implemented some :)

hwh33 · 2021-06-09T03:02:58Z

No problem! It's looking great. There are actually a number of additional comments. I think they might have been buried in the "hidden conversations" expander (see screenshot below). Probably on me for leaving so many 😅

oxtoacart · 2021-06-09T12:12:01Z

There are actually a number of additional comments

Thanks! Will look.

oxtoacart · 2021-06-09T14:32:09Z

@hwh33 Thanks for the awesome level of comments! I think I've provided answers to the remaining comments. Please let me know what you think.

oxtoacart · 2021-06-10T19:01:15Z

Okay, I've got more changes to make but I don't want to make this PR any bigger, so I'm going to go ahead and merge. If there's any issues with the code in here, we can open a new PR to address those.

hwh33 · 2021-06-11T22:25:59Z

Awesome work @oxtoacart!

oxtoacart added 30 commits March 11, 2021 21:22

Initial implementation of MessagingModel

17cd4ee

Automated generation of one time preKeys

508e60e

Progress on websocket client

443d622

Authentication to real tassis over websockets works

6094a99

Attempted with coroutines

b575f8a

Sort of working

5aaeb8c

Test succeeding and completing

fc6eb74

More complete test, but failing because of invalid signature on devic…

74539f5

…e key

Test passing

d892719

Test passing reliably

fa9061d

Progress on modeling

b986f33

Revised datamodel, implemented linter suggestions, reasonably good ha…

bc7da8c

…ppy path test

Better factoring of MessagingTest

c9a0779

Updated to latest db and secrets libraries

c57c74c

Ignore messages from senders who aren't in the Contact list

7a37566

Immediately create Conversation when adding Contact

7951a5c

Storing Base32 encoded versions of public and private identity keys i…

b5bc5af

…n database

More user friendly send API

ab6fafb

Removed ExperimentalTime dependency

b43c1f3

Updated to latest libsignal dependencies

234648d

Only update conversation timestamp if new message time is more recent…

afed114

… than previously recorded mostRecentMessageTime

Upgraded to latest secrets and db

b541864

Took out database contents logging in test

32a759f

Added ability to set my display name

6f6b42c

Simplified contact/conversation model

9a1c801

Rationalized naming

d486531

Enabled lost connection checking for websockets

a4f7676

Added ability to name database

df6d3e2

Improved concurrency

a7f5b9e

Cleaned up marking of failures

6cc45f9

atavism reviewed May 11, 2021

View reviewed changes

oxtoacart force-pushed the ox/newwebsocket branch from 2ca2c8e to 38ae0ef Compare May 11, 2021 21:27

Added introduction to README

48db04a

oxtoacart force-pushed the ox/newwebsocket branch from 38ae0ef to 48db04a Compare May 11, 2021 21:30

atavism reviewed May 17, 2021

View reviewed changes

messaging/src/main/java/io/lantern/messaging/metadata/Metadata.kt Show resolved Hide resolved

atavism reviewed May 17, 2021

View reviewed changes

messaging/src/main/java/io/lantern/messaging/metadata/Metadata.kt Show resolved Hide resolved

atavism reviewed May 17, 2021

View reviewed changes

oxtoacart added 3 commits May 24, 2021 13:45

Updated to latest db-android

8bb93a5

When remotely deleting messages, keep metadata around until user loca…

283a6e1

…lly deletes message

Recording timestamp when message was deleted by sender

787d74d

hwh33 reviewed Jun 8, 2021

View reviewed changes

oxtoacart added 3 commits June 8, 2021 07:26

Code review updates

4bb3a30

Updated to latest ktlint plugin

10cdbb5

Configured android flavor of ktlint

3976c36

Recording id of who remotely deleted a message

09ce412

oxtoacart force-pushed the ox/newwebsocket branch from 3fe6986 to 09ce412 Compare June 8, 2021 18:39

Reordered some fields in the model

9131aac

oxtoacart added 3 commits June 9, 2021 08:18

Using typed message for inbound messages

1d595a7

Removed ide config

17a9642

Code review updates

ba51ef3

oxtoacart merged commit b3f7ecf into main Jun 10, 2021

oxtoacart deleted the ox/newwebsocket branch June 10, 2021 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation #4

Initial implementation #4

oxtoacart commented Mar 26, 2021

atavism May 11, 2021 •

edited

Loading

oxtoacart May 11, 2021

atavism commented May 11, 2021

oxtoacart commented May 11, 2021

atavism May 17, 2021

oxtoacart May 17, 2021

hwh33 left a comment

hwh33 May 18, 2021

oxtoacart Jun 8, 2021

hwh33 Jun 9, 2021

hwh33 May 22, 2021

oxtoacart Jun 8, 2021

hwh33 Jun 9, 2021

oxtoacart Jun 9, 2021

oxtoacart commented Jun 8, 2021

hwh33 commented Jun 9, 2021

oxtoacart commented Jun 9, 2021

oxtoacart commented Jun 9, 2021

oxtoacart commented Jun 10, 2021

hwh33 commented Jun 11, 2021


		package tassis;

		option go_package = "github.com/getlantern/tassis/model";

Initial implementation #4

Initial implementation #4

Conversation

oxtoacart commented Mar 26, 2021

atavism May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atavism commented May 11, 2021

oxtoacart commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwh33 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxtoacart commented Jun 8, 2021

hwh33 commented Jun 9, 2021

oxtoacart commented Jun 9, 2021

oxtoacart commented Jun 9, 2021

oxtoacart commented Jun 10, 2021

hwh33 commented Jun 11, 2021

atavism May 11, 2021 •

edited

Loading