feat: Profile showcase data sharing #4209
Conversation
Jenkins BuildsClick to see older builds (115)
|
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
3 times, most recently
from
df4c721
to
a6da7c6
Compare
MishkaRogachev
requested review from
cammellos,
richard-ramos and
igor-sirotin
|
FYI: this PR contains temporary commit with protobuf binaries, see #4191 |
|
@saledjenic is it better to add synchronisation and backup for showcase in another PR? |
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
from
957acd7
to
e27e6f3
Compare
| m.allContacts.Range(func(_ string, contact *Contact) (shouldContinue bool) { | ||
| if contact.mutual() { | ||
| mutualContacts = append(mutualContacts, contact) | ||
| if contact.IsVerified() { |
There was a problem hiding this comment.
@MishkaRogachev isn't a verified contact at the same time the mutual contact as well? I guess we should be mutually exclusive here.
if contact.IsVerified() {
iDVerifiedContacts = append(iDVerifiedContacts, contact)
} else if contact.mutual() {
mutualContacts = append(mutualContacts, contact)
}
There was a problem hiding this comment.
If I understand the identity verification correctly, verifification avaliable only for mutual contacts, so here i explicitly ask for being mutual contacts for identity verified contacts:
| Visibility | For Everyone | For Contacts | For ID Verified Contacts |
|---|---|---|---|
| Everyone | true | false | false |
| Mutual contacts | true | true | false |
| Id verified mutual contacts | true | true | true |
There was a problem hiding this comment.
I agree, but I see that as verified contacts are a subset of mutual contacts, right?
But what I'm pointing you here is that in the current code, the same verified contact, let's say X, will be added to both arrays, mutualContacts and iDVerifiedContacts. It means that contact X will receive a single message meant to be used for mutual and another message meant to be used for verified contacts, that further means contact X will have a profile showcase for a user that is carried by the last received message, regardless is it mutual to verified contact.
Is that correct?
There was a problem hiding this comment.
if contact.IsVerified() {
iDVerifiedContacts = append(iDVerifiedContacts, contact)
} else if contact.mutual() {
mutualContacts = append(mutualContacts, contact)
}
With this implementation, a ID verified contact will be able to decrypt forIDVerifiedContacts part but will not be able to decrypt forContacts part because it won't contain his key. The test will fail here. So i will need to encrypt forContacts message both with iDVerifiedContacts and mutualContacts keys.
Each 'protobuf.ProfileShowcaseEntries' message contains only those items that are visible to the corresponding category. They are dispatched together in one protobuf.ProfileShowcase message, so end user will always use latest message, regardless is it mutual, verified or neither of them.
There was a problem hiding this comment.
@saledjenic I guess it's also about the contained data.
A verified contact should process all of the 3 categories to get full showcase. forIDVerifiedContacts only contains part of the showcase that's available to verified users.
| }, nil | ||
| } | ||
|
|
||
| func (m *Messenger) BuildProfileShowcaseFromIdentity(senderPubKey *ecdsa.PublicKey, message *protobuf.ProfileShowcase) (*identity.ProfileShowcase, error) { |
There was a problem hiding this comment.
@MishkaRogachev do we protect somehow that messages meant to be delivered to verified contacts can not be revealed by mutual contacts?
There was a problem hiding this comment.
Yes, each group encrypted with with explicitly specified contact's keys, same as in identity image flow
| package protobuf; | ||
|
|
||
| message ProfileShowcaseEntry { | ||
| string entry_id = 3; |
There was a problem hiding this comment.
@MishkaRogachev why don't we start from 1 here?
There was a problem hiding this comment.
it's typo, fixed :)
Whichever you prefer more. |
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
from
e27e6f3
to
f1dc7be
Compare
| result := []*protobuf.ProfileShowcaseEntry{} | ||
|
|
||
| for _, entry := range entries { | ||
| if entry.ShowcaseVisibility == visibility { |
There was a problem hiding this comment.
Maybe fast return here?
if entry.ShowcaseVisibility != visibility {
continue
}
// ...| m.allContacts.Range(func(_ string, contact *Contact) (shouldContinue bool) { | ||
| if contact.mutual() { | ||
| mutualContacts = append(mutualContacts, contact) | ||
| if contact.IsVerified() { |
There was a problem hiding this comment.
@saledjenic I guess it's also about the contained data.
A verified contact should process all of the 3 categories to get full showcase. forIDVerifiedContacts only contains part of the showcase that's available to verified users.
| _, err = WaitOnMessengerResponse( | ||
| theirMessenger, | ||
| func(r *MessengerResponse) bool { | ||
| return len(r.Contacts) > 0 && len(r.Messages()) > 0 && len(r.ActivityCenterNotifications()) > 0 |
There was a problem hiding this comment.
I think ActivityCenterNotifications shouldn't be part of wait condition, because it's created on the fly. We should check it afterwards, yes, but no during wait.
| } | ||
|
|
||
| func (s *MessengerProfileShowcaseSuite) TestSetAndGetProfileShowcasePreferences() { | ||
| func (s *TestMessengerProfileShowcase) prepareShocasePreferencs() ProfileShowcasePreferences { |
There was a problem hiding this comment.
2 typos combo! 😄
| func (s *TestMessengerProfileShowcase) prepareShocasePreferencs() ProfileShowcasePreferences { | |
| func (s *TestMessengerProfileShowcase) prepareShowcasePreferences() ProfileShowcasePreferences { |
| @@ -0,0 +1,9 @@ | |||
| DROP TABLE profile_showcase_contacts; | |||
|
|
|||
| CREATE TABLE IF NOT EXISTS profile_showcase_contacts ( | |||
There was a problem hiding this comment.
I guess IF NOT EXISTS is not needed if we drop the table just one line earlier 😄
|
@MishkaRogachev this is not related to storing the profile of ourselves, right? |
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
from
f1dc7be
to
f1f58c9
Compare
|
@saledjenic @igor-sirotin, thanks fort review!
No, as far as we don't use showcase data for user urls :) |
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
from
f1f58c9
to
3f232ee
Compare
protocol/identity_images.go
Outdated
| @@ -77,7 +77,7 @@ image: | |||
| // Decrypt the main encryption AES key with AES encryption using the DH key | |||
| dAESKey, err := common.Decrypt(empk, sharedKey) | |||
| if err != nil { | |||
| if err.Error() == "cipher: message authentication failed" { | |||
| if err.Error() == "cipher: message authentication failed" { // nolint: goconst | |||
There was a problem hiding this comment.
we should probably const this instead of disabling lint :)
protocol/persistence.go
Outdated
| @@ -727,6 +727,12 @@ func (db sqlitePersistence) Contacts() ([]*Contact, error) { | |||
| } | |||
| contact.SocialLinks = append(contact.SocialLinks, link) | |||
| } | |||
|
|
|||
| profileShowcase, err := db.GetProfileShowcaseForContact(contact.ID) | |||
There was a problem hiding this comment.
this probably should be a join for performance reasons? (same with social links) at least we could do
(1 for contacts, one for social links, one for profile showcase), instead of now that we do2*nContacts + 1 query, I think that's going to be slow maybe?
There was a problem hiding this comment.
The thing is, both for social links and profile entries, there are several rows for one contact id, so LEFT JOIN will trigger a whole query to have several rows. But challenge to optimise this func is interesting. Can I try to do it in separated PR ?
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
4 times, most recently
from
4eebe9a
to
5b2ab5b
Compare
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
2 times, most recently
from
f7adfc3
to
5c9c07f
Compare
|
@saledjenic @igor-sirotin @cammellos
I've decided to separate profile entries by category to be able provide custom fields |
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
from
d529279
to
cd8573f
Compare
| uint32 order = 5; | ||
| } | ||
|
|
||
| message ProfileShowcaseCollectible { |
There was a problem hiding this comment.
@dlipicar can you please check if there is a need to add any additional fields to assets and collectibles?
There was a problem hiding this comment.
not sure what the "uid" is. To identify any collectible you need three separate components:
- chainID
- contract address
- tokenID
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
3 times, most recently
from
6c985f4
to
8b359bd
Compare
There was a problem hiding this comment.
looks ok, main concern is performance of having to query multiple tables in separate queries, did you check though queries are covered by indexes? you are querying by contact-id most of the time, but primary key is composite, so depending on how the index is created (community first, contact last I guess), it could be slow if the community is popular, or viceversa if the user has many communities for example.
Also worth considering having a single table for your own? though no strong opinion on my side
| return m.persistence.GetProfileShowcasePreferences() | ||
| } | ||
|
|
||
| func (m *Messenger) EncryptProfileShowcaseEntriesWithContactPubKeys(entries *protobuf.ProfileShowcaseEntries, contacts []*Contact) (*protobuf.ProfileShowcaseEntriesEncrypted, error) { |
There was a problem hiding this comment.
The list of contacts might be quite big actually, maybe pass it by pointer?
There was a problem hiding this comment.
it's a slice, so it's already a pointer
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
2 times, most recently
from
c9df522
to
1f6681a
Compare
MishkaRogachev
force-pushed
the
feat/profile-showcase-messaging
branch
from
8837ff3
to
ca09eb0
Compare
Provide broadcasting of profile showcase changes for 3 categories: everyone, mutual contacts, and ID verified contacts.
Important changes:
Closes #4143