Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(decrypted_notify): update message payload and pass raw message #281

Merged
merged 5 commits into from
Nov 21, 2023
Merged

feat(decrypted_notify): update message payload and pass raw message #281

merged 5 commits into from
Nov 21, 2023

Conversation

geekbrother
Copy link
Contributor

@geekbrother geekbrother commented Nov 16, 2023 •
edited
Loading

Description

This PR adds topic, tag and message fields for the send notification request payload to follow the specs for the decrypted notifications.

The handler modification checks if the client was registered (have) always_raw as true and then handles the message as an encrypted with the payload from the message field. Otherwise, handle the payload as usual to support backward compatibility.

Related to #278

How Has This Been Tested?

  • Current clients support is tested by the current unit/integration tests (passing current tests on new changes),
  • New clients with always_raw = true and push message with the new required fields are tested by the new test_push_always_raw test.

Review stack 🏗️

1 - Adding always_raw for the client registration #279
2 - Update message payload and pass raw message #281 <-

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@geekbrother geekbrother self-assigned this Nov 16, 2023
@arein arein added the accepted The issue has been accepted into the project label Nov 16, 2023
@geekbrother geekbrother mentioned this pull request Nov 16, 2023
Merged
3 tasks
@geekbrother geekbrother changed the base branch from main to feat/decrypted_notifs/add_always_raw_to_register November 16, 2023 22:42
@geekbrother geekbrother force-pushed the feat/decrypted_notifs/add_always_raw_to_register branch from fa9b0b9 to 8269b2c Compare November 16, 2023 22:49
@geekbrother geekbrother force-pushed the feat/decrypted_notifs/update_message_payload_and_pass_raw branch 2 times, most recently from 1742104 to 4a6f0c3 Compare November 16, 2023 23:04
@geekbrother geekbrother force-pushed the feat/decrypted_notifs/add_always_raw_to_register branch from 8269b2c to ed6a83e Compare November 16, 2023 23:24
chris13524
chris13524 requested changes Nov 16, 2023
View reviewed changes
src/providers/apns.rs Outdated Show resolved Hide resolved
src/providers/fcm.rs Outdated Show resolved Hide resolved
@geekbrother geekbrother force-pushed the feat/decrypted_notifs/update_message_payload_and_pass_raw branch 2 times, most recently from bca5560 to 3a9a2a0 Compare November 17, 2023 00:33
@geekbrother geekbrother marked this pull request as ready for review November 17, 2023 00:47
chris13524
chris13524 approved these changes Nov 17, 2023
View reviewed changes
Copy link
Member

@chris13524 chris13524 left a comment

Choose a reason for hiding this comment

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

LGTM, but please do not merge until https://github.com/WalletConnect/rs-relay/pull/1285 is merged so we can ensure there are no backwards incompatibilities.

src/providers/fcm.rs Outdated Show resolved Hide resolved
src/providers/fcm.rs Outdated Show resolved Hide resolved
@chris13524 chris13524 requested a review from xav November 17, 2023 01:07
@geekbrother geekbrother force-pushed the feat/decrypted_notifs/update_message_payload_and_pass_raw branch from 3a9a2a0 to 28f331e Compare November 17, 2023 12:28
@chris13524 chris13524 mentioned this pull request Nov 17, 2023
Merged
3 tasks
geekbrother
geekbrother commented Nov 20, 2023
View reviewed changes
src/handlers/push_message.rs
@@ -175,18 +178,52 @@ pub async fn handler_internal(
)
})?;

let cloned_body = body.clone();
let push_message = if client.always_raw {
Copy link
Contributor Author

@geekbrother geekbrother Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Probably this condition or our spec description should be changed because in our spec we require the tag to be presented as well if always_raw == false to filter by tag and send clear text notification for some of the tags. I think we should fix this in a spec and do tag filtering when the always_raw == true. cc @chris13524

Copy link
Member

Choose a reason for hiding this comment

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

When set to false, cleartext notifications will be sent for certain messages based on tag

This filtering is performed in the relay, and was already implemented. So I don't think anything needs to be done here.

xav
xav approved these changes Nov 21, 2023
View reviewed changes
Cargo.toml Outdated
@@ -66,7 +66,7 @@ fcm = "0.9"
ed25519-dalek = "2.0.0-rc.2"

# JWT Authentication
relay_rpc = { git = "https://github.com/WalletConnect/WalletConnectRust.git", rev = "ced99e7"}
relay_rpc = { git = "https://github.com/WalletConnect/WalletConnectRust.git", rev = "4ee9007"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Must we use hashes on repos we control?
It would be much better to make a release and use the tag in our dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

I switched it 👍

Base automatically changed from feat/decrypted_notifs/add_always_raw_to_register to main November 21, 2023 16:37
@geekbrother geekbrother force-pushed the feat/decrypted_notifs/update_message_payload_and_pass_raw branch from c22be2a to d79dbf3 Compare November 21, 2023 16:43
@geekbrother geekbrother merged commit 3990b70 into main Nov 21, 2023
6 checks passed
@geekbrother geekbrother deleted the feat/decrypted_notifs/update_message_payload_and_pass_raw branch November 21, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris13524 chris13524 chris13524 approved these changes

@xav xav xav approved these changes

Assignees

@geekbrother geekbrother

Labels
accepted The issue has been accepted into the project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@geekbrother @xav @chris13524 @arein