Push notification implementation #1997
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 (201)
|
cammellos
force-pushed
the
feature/mute-chats-2
branch
from
b2830b9 to
ccd3234
Compare
cammellos
force-pushed
the
feature/mute-chats-2
branch
4 times, most recently
from
faa1266 to
757b027
Compare
cammellos
force-pushed
the
feature/mute-chats-2
branch
from
1b49065 to
619ac01
Compare
cammellos
force-pushed
the
feature/mute-chats-2
branch
2 times, most recently
from
b49f0c4 to
45a5fe4
Compare
|
@Samyoul @jakubgs @andremedeiros I have updated the specs to the latest changes status-im/specs#146 |
cammellos
force-pushed
the
feature/mute-chats-2
branch
from
5ca2c39 to
08dc687
Compare
andremedeiros
approved these changes
Jul 23, 2020
multiaccounts/accounts/database.go
Outdated
| PreviewPrivacy bool `json:"preview-privacy?"` | ||
| PublicKey string `json:"public-key"` | ||
| // PushNotificationServerEnabled indicates whether we should be running a push notification server | ||
| PushNotificationsServerEnabled bool `json:"push-notifications-server-enabled,omitempty"` |
There was a problem hiding this comment.
We're not being consistent on how we name boolean fields. See above with notifications-enabled?. Can we decide on one and stay consistent?
There was a problem hiding this comment.
I think we should rename everything to camelCase, as now there's also desktop and these are all clojurisms, but I will make it consistent for now
| ctx context.Context, | ||
| recipient *ecdsa.PublicKey, | ||
| rawMessage *RawMessage, | ||
| ) ([]byte, error) { | ||
| p.logger.Debug( | ||
| "sending a private message", | ||
| zap.Binary("public-key", crypto.FromECDSAPub(recipient)), | ||
| zap.String("public-key", types.EncodeHex(crypto.FromECDSAPub(recipient))), |
| // Currently we don't support sending through datasync and setting custom waku fields, | ||
| // as the datasync interface is not rich enough to propagate that information, so we | ||
| // would have to add some complexity to handle this. | ||
| if rawMessage.ResendAutomatically && (rawMessage.Sender != nil || rawMessage.SkipEncryption) { |
There was a problem hiding this comment.
Should we check for p.features.Datasync here too?
| ctx context.Context, | ||
| recipient *ecdsa.PublicKey, | ||
| rawMessage *RawMessage, | ||
| ) ([]byte, error) { | ||
| p.logger.Debug("sending private message", zap.Binary("recipient", crypto.FromECDSAPub(recipient))) | ||
| p.logger.Debug("sending private message", zap.String("recipient", types.EncodeHex(crypto.FromECDSAPub(recipient)))) |
| err := message.HandleEncryption(decryptionKey, publicKey, p.protocol, skipNegotiation) | ||
|
|
||
| // if it's an ephemeral key, we don't have to handle a device not found error | ||
| if err == encryption.ErrDeviceNotFound && !skipNegotiation { |
There was a problem hiding this comment.
I'm having a hard time seeing where this error comes from. In fact, this method only returns wrapped errors, which leads me to believe that the left condition would never evaluate to true.
cammellos
force-pushed
the
feature/mute-chats-2
branch
3 times, most recently
from
cbf6ef8 to
b514b44
Compare
cammellos
force-pushed
the
feature/mute-chats-2
branch
from
ef9fb43 to
4b8739a
Compare
|
I will be moving to work on emojii reactions, I will merge this and address the feedback in a separate PR. |
cammellos
force-pushed
the
feature/mute-chats-2
branch
2 times, most recently
from
54ce2c0 to
c61bf0c
Compare
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.
Implements push notification server & client.
The way it is implemented is that any client can just act as a push notification server, so you can run this on your android phone if you wish, as it's just a waku/whisper client.
I have heavily commented the code as the functioning is rather complex, the server is pretty straightforward, while the client is quite complicated.
Is not quite finished, there's still some things to handle and improved, but it's at a stage that is ready to be integrated.
There are full e2e tests of the functionalities in
protocol/push_notification_test.go.I have left the commit split and committed often to ease the review process, it's quite a lot of code unfortunately.
I haven't quite updated the specs with the latest changes, I will do that tomorrow, but most of it is there: https://github.com/status-im/specs/blob/master/docs/raw/push-notification-server.md
UPDATE: specs PR status-im/specs#146
Things that have also changed:
common