Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

71/STATUS-Push-Notification-Server : add new RFC #629

Closed
wants to merge 77 commits into from

Conversation

jimstir
Copy link
Contributor

@jimstir jimstir commented Nov 1, 2023

No description provided.

@jimstir jimstir requested review from kaiserd and rymnc November 1, 2023 05:36
Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your first PR! 🚀
In general lgtm, certain semantic changes are required!

content/docs/rfcs/71/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/71/README.md Show resolved Hide resolved
content/docs/rfcs/71/README.md Outdated Show resolved Hide resolved
Comment on lines 38 to 39
mailserver
> A Waku node that provides functionality to store messages permanently and deliver the messages to requesting clients.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont believe we use mailservers anymore cc: @felicio

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That should be removed. We could add store nodes if that makes sense in the scope of this document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced mailserver with Waku-Store.

content/docs/rfcs/71/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/71/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/71/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/71/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/71/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/71/README.md Outdated Show resolved Hide resolved
jimstir and others added 8 commits November 1, 2023 13:39
Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>
Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>
Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>
Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>
Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>
Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>
Copy link
Contributor

@kaiserd kaiserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the progress so far. A few minor inline comments:

## Push Notification Server Flow
### Registration Process:

![image](https://github.com/jimstir/rfc/assets/91767824/00cae4bf-6eb5-4e7b-9377-3a5d01b041a1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move this ressources to this github repo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump: This ref still has to be updated. Link similar to the spec links, relative to the static website, not to github.
(You can check other RFCs for reference.)

content/docs/rfcs/71/README.md Outdated Show resolved Hide resolved
### Handle Errors:
- If the message can’t be decrypted, the message MUST be discarded.
- If `token_type` is not supported, a response MUST be sent with `error` set to `UNSUPPORTED_TOKEN_TYPE`.
-If `token`, `installation_id`, `device_tokens`, `version` are empty, a response MUST be sent with `error` set to `MALFORMED_MESSAGE`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-If `token`, `installation_id`, `device_tokens`, `version` are empty, a response MUST be sent with `error` set to `MALFORMED_MESSAGE`.
- If `token`, `installation_id`, `device_tokens`, `version` are empty, a response MUST be sent with `error` set to `MALFORMED_MESSAGE`.

@kaiserd
Copy link
Contributor

kaiserd commented Nov 10, 2023

@jimstir There seems to be a problem with the directory structure.
You added a waku-keystore.md to both this PR and your new Waku key-store PR.
It seems you are only using one local branch for both, or you do not have the branches properly separated.
Also, please add the new RFC to the index: https://github.com/vacp2p/rfc/blob/master/content/menu/index.md

@kaiserd
Copy link
Contributor

kaiserd commented Dec 11, 2023

Closed. Git history was messed up in this PR.
New PR: #643

@kaiserd kaiserd closed this Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants