Add spec for the contact request refactor #154
Conversation
There was a problem hiding this comment.
Overall looks good, nice work and thanks for writing this up!
I have some questions/comments about the ContactUpdate section.
The spec is a bit implementation heavy (we probably don't want to talk about tables etc, as clients might use different databases for example), but rather just behaviors.
docs/draft/6-payloads.md
Outdated
| @@ -275,6 +288,16 @@ Messages with a `clock` less than `120` seconds under the Whisper/Waku timestamp | |||
|
|
|||
| The node uses `clock` value for the message ordering. The algorithm used, and the distributed nature of the system produces casual ordering, which might produce counter-intuitive results in some edge cases. For example, when a user joins a public chat and sends a message before receiving the exist messages, their message `clock` value might be lower and the message will end up in the past when the historical messages are fetched. | |||
|
|
|||
| #### Signature | |||
There was a problem hiding this comment.
I think we need to define what the signature is composed of.
Basically we need to prove that a user really wanted to add another user to contacts.
So something like Sign(Keccak256(PK1 + PK2 + some custom bytes to make sure the signature can't be reused for a differet purpose)
Also probably we should name the field something a bit more meaningful
docs/draft/6-payloads.md
Outdated
| | Field | Name | Type | Description | | ||
| | ----- | ---- | ---- | ---- | | ||
| | 1 | id | `string` | ID of the contact. Hex-encoded public key (prefixed with 0x). | | ||
| | 2 | address | `string` | Ethereum address of the contact | |
There was a problem hiding this comment.
I think this is a legacy field, we used to have it once wallet was derived by the public key, but in case pk is enough.
docs/draft/6-payloads.md
Outdated
| | 3 | name | `string` | Contact's ENS name | | ||
| | 4 | ensVerified | `bool` | Whether the name of the contact is verified | | ||
| | 5 | alias | `string` | Contact's generated username | | ||
| | 6 | identicon | `string` | Identicon generated from public key | |
There was a problem hiding this comment.
No need to send this over, as it can be generated by the pk
docs/draft/6-payloads.md
Outdated
| | 9 | added | `bool` | Whether the contact is added | | ||
| | 10 | requestReceived | `bool` | Whether the contact requested us to add them | | ||
| | 11 | mutualContact | `bool` | Whether the contact is mutual | | ||
| | 12 | deviceInfo | `[]ContactDeviceInfo` | | |
There was a problem hiding this comment.
This might also be legacy and not used anymore
docs/draft/6-payloads.md
Outdated
| | 2 | address | `string` | Ethereum address of the contact | | ||
| | 3 | name | `string` | Contact's ENS name | | ||
| | 4 | ensVerified | `bool` | Whether the name of the contact is verified | | ||
| | 5 | alias | `string` | Contact's generated username | |
There was a problem hiding this comment.
No need to send this over as generated by the pk
docs/draft/6-payloads.md
Outdated
| `Contact` is a representation of other accounts encountered on the app. They are added through `ContactUpdate`s propagated. | ||
| A `Contact` does not mean a "friend". Any account from which we receive a `ContactUpdate` is added to the table. | ||
|
|
||
| Users are considered "Friends" when they are **mutual** contacts, meaning that both of them have the `added` and `requestReceived` boolean set as `true`, which then triggers setting `mutualContact` to `true`. This is done through `ContactRequest`s. |
There was a problem hiding this comment.
When is this sent exactly?
the payload below looks ok if it's to be sent to your devices for syncing contacts, but otherwise is too many info for sending to other users.
|
Is this PR dead? |
cammellos
force-pushed
the
feature/contact-requests
branch
2 times, most recently
from
52209af
to
de58fbf
Compare
jrainville
force-pushed
the
feature/contact-requests
branch
from
de58fbf
to
60870f9
Compare
|
@cammellos sorry for the late reply. I was on vacation when you did the changes so I missed it and before that, this feature was not a priority so I had pushed it back. Anyway, I rebased the PR, fixed a typo and also reviewed your changes. I can understand if it is the latter, since it would have been heavy on the network and also harder to implement. The contact signature is easier and also does solve the 1-1 chat issues we have. |
|
@jrainville
We have now implemented contact backup, it hasn't been specced yet, but it's already in develop (and likely in desktop), and it works slightly differently, so I thought I'd remove it |
|
Turns out I can't approve my own PR 😅 |
Specs Contacts, the contact requests and the way to persist them on the network.