Partitioned topic and topic negotiation #8363
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (202)
|
yenda
reviewed
Jun 5, 2019
| } | ||
|
|
||
| @ReactMethod | ||
| public void removeFilter(final String chat, final Callback callback) { |
There was a problem hiding this comment.
why did you chose to do this in native rather than using rpc?
There was a problem hiding this comment.
yes, I went initially for native, I got my signals crossed, I thought that was what we wanted, but it's on RPC already, I will remove this
| @@ -169,22 +169,16 @@ | |||
| (log/error "can't remove a chat:" error))}]} | |||
| (navigation/navigate-to-cofx :home {})) | |||
| (fx/merge cofx | |||
| ;; TODO: There's a race condition here, as the removal of the filter (async) | |||
src/status_im/contact_code/core.cljs
Outdated
| @@ -17,9 +17,9 @@ | |||
| (str pk "-contact-code")) | |||
|
|
|||
| (fx/defn listen [cofx chat-id] | |||
| (transport.public-chat/join-public-chat | |||
| (transport.public-chat/join-one-to-one-chat | |||
There was a problem hiding this comment.
that's odd why is join one-to-one in public-chat namespace
There was a problem hiding this comment.
oh it's all still WIP, I just did not want to modify join-public-chat for now
src/status_im/mailserver/core.cljs
Outdated
| @@ -34,7 +34,8 @@ | |||
| ;; as soon as the mailserver becomes available | |||
|
|
|||
|
|
|||
| (def one-day (* 24 3600)) | |||
| (def one-hour 3600) | |||
| @@ -152,6 +152,30 @@ | |||
| (when (status) | |||
| (.disableInstallation (status) installation-id callback))) | |||
|
|
|||
| (defn load-filters [chats callback] | |||
There was a problem hiding this comment.
ok here it is rpc calls so the native code above is going to be removed?
src/status_im/transport/filters.cljs
Outdated
| (re-frame/dispatch | ||
| [:shh.callback/filters-added | ||
| (keep | ||
| (fn [{:keys [options callback chat-id]}] | ||
| (when-let [filter (.newMessageFilter | ||
| (when-let [filter (.newRawMessageFilter |
There was a problem hiding this comment.
seems like the last use of web3 could you use json rpc here so we can finaly get rid of it?
There was a problem hiding this comment.
this one I need for the polling (I just need the Filter js object), it actually does not make any request through RPC, but we can get rid of it all together once we use signals for messages, once we get closer to completion of the PR we can revisit and we can decide whether to include it in this one or a separate, depending on how much stuff there is to test
|
|
cammellos
force-pushed
the
features/partitioned-topic
branch
from
aa065a1
to
d203fab
Compare
|
|
8 similar comments
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
cammellos
force-pushed
the
features/partitioned-topic
branch
from
9b894a9
to
5de283f
Compare
|
|
1 similar comment
|
|
cammellos
force-pushed
the
features/partitioned-topic
branch
from
5de283f
to
054f294
Compare
|
|
1 similar comment
|
|
cammellos
force-pushed
the
features/partitioned-topic
branch
from
0c2c0ee
to
0563d86
Compare
|
|
cammellos
force-pushed
the
features/partitioned-topic
branch
from
0563d86
to
79e4044
Compare
|
|
cammellos
force-pushed
the
features/partitioned-topic
branch
from
79e4044
to
62546e6
Compare
|
|
|
@churik it should be ready to be tested again
The message appears as soon as we detect some activity from another device, so it might happen before you click on Advertise. I could not replicate 8, it might be due to a dropped message, currently we don't resend them. There's an outstanding issue on upgrade that on slow devices it might take some time (1/2 minutes, depending on how many public-chat/one-to-one), during this time the app is usable, but no messages are sent/received. After the initial upgrade everything should be working fast (this is due to the fact that we have to generate some keys the first time around). I have also reverted the short interval for gaps for easier testing, feel free to re-introduce it. Thanks for your patience |
|
@cammellos about recent changes: as I understand, I should retest once more:
|
|
yes, that is correct.
The feature itself (from the logs), is not that important as of now (I have
checked many times, and I am more concerned about potential bugs), but of
course if you have time it s all good.
thanks
…On Fri, Jun 21, 2019, 09:35 Tetiana Churikova ***@***.***> wrote:
@cammellos <https://github.com/cammellos> about recent changes: as I
understand, I should retest once more:
- upgrade
- the issues are not reproducible
- and the feature itself (from the description).
Is it right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8363>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHYJMDNBXXUPBND2JKLZBDP3SAFNANCNFSM4HTWA5KA>
.
|
|
@cammellos Issue 8: If you add to contact via 1-1 chat after syncing, new contact doesn't appear on paired deviceSteps:
Expected result: contact is added on both devices "Sync all" on deviceA doesn't help. Logs from deviceA: Status-debug-logsA.zip Logs from deviceB: status_logsB.zip |
|
thanks, i will take another look at it tonight, so probably will be ready
on Monday as i am off today. have a good weekend!
…On Fri, Jun 21, 2019, 11:41 Tetiana Churikova ***@***.***> wrote:
@cammellos <https://github.com/cammellos>
Issue 7 is fixed.
Issue 8 is still reproducible, more detailed steps are below (sorry if
they weren't clear):
Issue 8: If you add to contact via 1-1 chat after syncing, new contact
doesn't appear on paired device
*Steps:*
- create accountA on deviceA
- restore accountA on deviceB
- deviceA: start 1-1 chat with UserC, send several messages to UserC
(don't add UserC to contacts)
So "Add to contacts" should be visible in 1-1 chat.
- deviceB: advertise deviceB to deviceA
- deviceA: enable deviceB in "Devices"
- deviceA: tap on sync all
- deviceA: send message to 1-1 chat with UserC. It should appear on
deviceB.
- deviceA : tap on "Add to contacts" in 1-1 chat with UserC
*Expected result:* contact is added on both devices
*Actual result:* contact is added only on deviceA.
"Sync all" on deviceA doesn't help.
All fine if I add new contact on deviceA from public chat - it appears on
deviceB.
*Logs from deviceA:* Status-debug-logsA.zip
<https://github.com/status-im/status-react/files/3313756/Status-debug-logsA.zip>
*Logs from deviceB:* status_logsB.zip
<https://github.com/status-im/status-react/files/3313758/status_logsB.zip>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8363>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHYJMGGEZOHNCPZAOOK3V3P3SO23ANCNFSM4HTWA5KA>
.
|
|
|
|
@churik thanks for the repro steps, managed to replicate. I believe the issue is also present in develop, and likely was introduced either with the introduction of system-tags or tribute to talk, as essentially system tags have changed the way we represent a contact, while ttt made some changes on when we store a record (opening a chat will add a "contact" to a list internally, which was then synced when pressing There might still be some issues lurking around contacts & syncing, but probably best to fix separately. |
cammellos
force-pushed
the
features/partitioned-topic
branch
from
ad953b9
to
252ab1a
Compare
|
|
|
Retested on previous builds:
Retested on last build (252ab1a):
Looks good for me. |
|
awesome, thanks a lot for your help and patience @churik ! |
cammellos
force-pushed
the
features/partitioned-topic
branch
from
252ab1a
to
57a4a60
Compare
All the code has been implemented in statusgo: status-im/status-go#1466 Basically all the whisper filter management is done at that level. Technical description On startup we load all chats and send a list of them to status go: For a public chat: {:chatId "status"}, we create a single filter, based on the name of the chat. For each contact added by us, each user in a group chat and each one to one chat open, we send: {:chatId "0x", :oneToOne true}. This will create a chats, to listen to their contact code. Any previously negotiated topic is also returned. Once loaded, we create our filters, and upsert the mailserver topics, both of which are solely based on the filters loaded. In order to remove a chat, we delete/stopwatching first the the filter in status-react and then ask status-go to remove the filter. For a public chat we always remove, for a one-to-one we remove only if the user is not in our contacts, or in a group chat or we have a chat open. Negotiated topics are never removed, as otherwise the other user won't be able to contact us anymore. On stopping whisper we don't have to ask status-go to remove filters as they are removed automatically. Some more logic can be pushed in status-go, but that will be in subsequent PRs. Signed-off-by: Andrea Maria Piana <andrea.maria.piana@gmail.com>
cammellos
force-pushed
the
features/partitioned-topic
branch
from
57a4a60
to
532664a
Compare
This was referenced Jun 24, 2019
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds topic negotiation and partitioned topic for group-chats and pairing.
All the code has been implemented in statusgo: status-im/status-go#1466
Basically all the whisper filter management is done at that level.
Technical description
For a public chat:
{:chatId "status"}, we create a single filter, based on the name of the chat.For each contact added by us, each user in a group chat and each one to one chat open, we send:
{:chatId "0x", :oneToOne true}. This will create a chats, to listen to their contact code.Any previously negotiated topic is also returned.
In order to remove a chat, we delete/stopwatching first the the filter in status-react and then ask status-go to remove the filter. For a public chat we always remove, for a one-to-one we remove only if the user is not in our contacts, or in a group chat or we have a chat open. Negotiated topics are never removed, as otherwise the other user won't be able to contact us anymore.
On stopping whisper we don't have to ask status-go to remove filters as they are removed automatically.
Some more logic can be pushed in status-go, but that will be in subsequent PRs.
Testing
This is a fairly large change and affects quite a large part of the codebase.
Specifically:
8)Performance just after login (between login and plus icon is visible), it should be slightly faster if anything but roughly there should be no difference
Everything should work just in the same way.
To test the actual feature you will have check
geth.log, in Debug level, for lines.Sending on negotiated topicSending on partitioned topicSending on old discovery topicAnd also you need to make sure the public key printed after is the one for the right contact (we also send pairing messages, to your own public key).
How it works
It's all based on device-to-device communications, so make sure you pair your devices before testing (might work even if you don't pair them), and that you don't have more than 3 devices on each side.
The idea is that given a group of devices A1,A2,A3 you will send on:
old discovery topicthat happens if 1) if you don't have information about their devices (no communication happened before) or 2) one of those devices is running an older version.partitioned topicif all 3 devices are running this version, but you have not send a message back from each one of those devices.negotiated topicif all 3 devices are running this version and you have sent a message back from each one of those devices.Examples
1 device on each side
A1 running this version, B1 running this version:
Send messages from A1, (don't send any message from B1) you should see either:
Sending on old discovery topicorSending on partitioned topic. The more you wait with both devices online, the more likely you will start seeingSending on partitioned topic. (This is due to the fact that devices advertise themselves periodically, so it might take some time to detect each other).Once you reply from B1 you should see
Sending on negotiated topicon both devices when sending a message.A1 running this version, B1 running an older version
You should always see
Sending on old discovery topicwhen sending messages from A1 (B1 will not have those logs as running an older version).1 device on one side, 2 on the other
A1 running this version, B1 , B2 running this version.
Pair B1 & B2, sending from A1 should be either
Sending on old discovery topicorSending on partitioned topic.Send a message from B1, you should see
Send on negotiated topic.Send a message from A1 you should see
Send on partitioned topic.Send a message from B2 you should see
Send on negotiated topic.Send a message from A1 you should see
Send on negotiated topic.A1 running this version, B1 running this version B2 running an older version.
Pair B1 & B2, sending from A1 should be either
Sending on old discovery topicorSending on partitioned topic.Send a message from B1, you should see
Send on negotiated topic.Send a message from A1 you should see
Send on old discovery topic.Send a message from B2 (no logs as an older version).
Send a message from A1 you should see
Send on old discovery topic.If a device is running this version, and sending a message to a user that has at least one device running an older version, you should always see:
Sending on old discovery topic.Basically if all the devices you are sending the message to are running this version
apologies if it's not clear, feel free to ask any question.
To test using normal one to one messages (easier) you can enable
device-to-devicein the settings.status: ready