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.

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

test(waku-relay): Relay #2101

Merged
merged 17 commits into from
Nov 15, 2023
Merged

test(waku-relay): Relay #2101

merged 17 commits into from
Nov 15, 2023

Conversation

AlejandroCabeza
Copy link
Contributor

@AlejandroCabeza AlejandroCabeza commented Oct 3, 2023

Description

Implement waku filter relay tests for message id generation

Changes

  • Implement message id tests.
  • Implement relay tests.
  • Update import paths to use test_all.

Dependencies

#2096

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2101

Built from f5c995c

tests/waku_relay/test_protocol.nim Outdated Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Outdated Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Show resolved Hide resolved
@AlejandroCabeza AlejandroCabeza mentioned this pull request Oct 18, 2023
3 tasks
@AlejandroCabeza AlejandroCabeza changed the title test(waku-filter): Relay message id test(waku-filter): Relay Oct 20, 2023
@AlejandroCabeza AlejandroCabeza requested review from vpavlin and removed request for Ivansete-status October 30, 2023 14:40
import nimcrypto


proc cfbEncode*(key: string, iv: string, data: string): seq[byte] =
Copy link
Member

Choose a reason for hiding this comment

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

Why is this called encode if it actually encrypts ? (same for the proc below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were sourced (and slightly adapted) from nimcrypto/examples/cfb.nim, where they used the encode and decode keywords in the respective variables. Thus, I assumed those keywords would be understood.

let fromOtherWakuMessage = fakeWakuMessage("fromOther")
discard await node.publish(pubsubTopic, fromOtherWakuMessage)

# Then the message is published only in both nodes
Copy link
Member

Choose a reason for hiding this comment

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

I think the comment has a copy&paste error:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed 😅 You repeat one thing too many times and you start missing obvious errors


# When decrypting the message
let
decryptedText = cfbDecode(key, iv, msg1.payload)
Copy link
Member

Choose a reason for hiding this comment

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

Slightly inconsistent - we have encodedText and decryptedText

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, updating :)

@vpavlin
Copy link
Member

vpavlin commented Nov 1, 2023

I am mildly confused about this PR - the title say waku-filter but all the tests are testing relay

@AlejandroCabeza
Copy link
Contributor Author

I am mildly confused about this PR - the title say waku-filter but all the tests are testing relay

That was a typo on my side, updating it.

@AlejandroCabeza AlejandroCabeza changed the title test(waku-filter): Relay test(waku-relay): Relay Nov 1, 2023
tests/waku_filter_v2/test_all.nim Show resolved Hide resolved
tests/waku_relay/test_message_id.nim Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Outdated Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Outdated Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Thanks so much, looks great! :)
Left some comments/questions

tests/waku_relay/test_protocol.nim Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Outdated Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Outdated Show resolved Hide resolved
tests/waku_relay/test_protocol.nim Outdated Show resolved Hide resolved
@gabrielmer gabrielmer self-requested a review November 8, 2023 08:45
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Member

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from test-waku-filter-node-e2e to test-waku-filter-unsubscribe-all November 15, 2023 09:24
Base automatically changed from test-waku-filter-unsubscribe-all to test-waku-filter-unsubscribe November 15, 2023 09:25
Base automatically changed from test-waku-filter-unsubscribe to master November 15, 2023 09:26
@AlejandroCabeza AlejandroCabeza marked this pull request as ready for review November 15, 2023 15:11
@AlejandroCabeza AlejandroCabeza merged commit f5f4313 into master Nov 15, 2023
6 of 8 checks passed
@AlejandroCabeza AlejandroCabeza deleted the test-relay-subscribe branch November 15, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants