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

Relay request and response additions #151

Merged
merged 7 commits into from
May 19, 2024

Conversation

tyiu
Copy link
Contributor

@tyiu tyiu commented May 2, 2024

Please review this PR on a per-commit basis. Otherwise, it will be difficult to review.

  • Reorder relay request and relay response enums to match the listed ordering in the specs for easier maintainability
  • Fix RelayResponse to not drop human readable messages for OK messages
  • Add CLOSED relay response
  • Add AUTH relay request and response

This PR brings RelayRequest and RelayResponse to completion per NIP-01, NIP-42 - Auth, and NIP-45 - Event counts. The implementation was previously incomplete, covering only part of the spec.

Although this PR adds AUTH, there are no functions to actually authenticate on the RelayOperating protocol yet. That will be added as a separate PR.
This issue tracks the AUTH work: #21

@tyiu tyiu requested a review from bryanmontz May 2, 2024 05:24
@tyiu
Copy link
Contributor Author

tyiu commented May 2, 2024

There's something wrong with the CI builds. I'm getting flaky results or they're breaking in a way that's unexpected. It seems that my PR shouldn't affect them, but maybe I'm missing something.

@tyiu
Copy link
Contributor Author

tyiu commented May 4, 2024

There's something wrong with the CI builds. I'm getting flaky results or they're breaking in a way that's unexpected. It seems that my PR shouldn't affect them, but maybe I'm missing something.

It's not my PR. I tried the build on a different branch that's basically just the current main branch and it fails in the same way for the Swift 5.7.1 build. Either the tooling or environment has changed.

Run swift build
  swift build
  swift test
  shell: /bin/bash -e {0}
  env:
    TOOLCHAINS: org.swift.571202211011a
error: 'nostr-sdk-ios': Invalid manifest
error: compile command failed due to signal 6 (use -v to see invocation)
/Applications/Xcode_15.0.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/usr/lib/swift/Dispatch.swiftmodule/arm64e-apple-macos.swiftinterface:84:20: error: no type named 'DispatchQueue' in module 'Dispatch'
extension Dispatch.DispatchQueue {
                   ^

@tyiu
Copy link
Contributor Author

tyiu commented May 5, 2024

There's something wrong with the CI builds. I'm getting flaky results or they're breaking in a way that's unexpected. It seems that my PR shouldn't affect them, but maybe I'm missing something.

It's not my PR. I tried the build on a different branch that's basically just the current main branch and it fails in the same way for the Swift 5.7.1 build. Either the tooling or environment has changed.

Run swift build
  swift build
  swift test
  shell: /bin/bash -e {0}
  env:
    TOOLCHAINS: org.swift.571202211011a
error: 'nostr-sdk-ios': Invalid manifest
error: compile command failed due to signal 6 (use -v to see invocation)
/Applications/Xcode_15.0.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/usr/lib/swift/Dispatch.swiftmodule/arm64e-apple-macos.swiftinterface:84:20: error: no type named 'DispatchQueue' in module 'Dispatch'
extension Dispatch.DispatchQueue {
                   ^

#152 fixes the build issue.

@tyiu tyiu force-pushed the tyiu/add-relay-request-response branch from 5f5770a to 8f7c74e Compare May 5, 2024 02:22
@tyiu tyiu requested a review from joelklabo May 5, 2024 02:23
Copy link
Collaborator

@bryanmontz bryanmontz left a comment

Choose a reason for hiding this comment

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

Thanks, Terry!

Sources/NostrSDK/Events/AuthenticationEvent.swift Outdated Show resolved Hide resolved
Sources/NostrSDK/RelayResponse.swift Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
["OK", "b1a649ebe8b435ec71d3784793f3bbf4b93e64e17568a741aecd4c7ddeafce30", true, "unknown: reason: unknown"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible that the message will have two colons in it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's possible. Technically the relay can give us whatever it wants and not conform to NIP-01. I wanted to stress test that possibility.

Tests/NostrSDKTests/RelayResponseDecodingTests.swift Outdated Show resolved Hide resolved
@tyiu tyiu requested a review from bryanmontz May 19, 2024 12:06
@bryanmontz bryanmontz merged commit 951f1fe into main May 19, 2024
4 checks passed
@bryanmontz bryanmontz deleted the tyiu/add-relay-request-response branch May 19, 2024 13:31
RandyMcMillan pushed a commit to RandyMcMillan/nostr-sdk-ios that referenced this pull request Sep 1, 2024
* Reorder relay request and relay response enums to match the listed ordering in the specs for easier maintainability

* Fix RelayResponse to not drop human readable messages for OK messages

* Add CLOSED relay response

* Add AUTH relay request and response

* Apply suggestions from code review

Co-authored-by: Bryan Montz <[email protected]>

* Shorten struct names

* Change RelayResponseDecodingTest to use XCTUnwrap instead of if let to make the test code cleaner

---------

Co-authored-by: Bryan Montz <[email protected]>
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.

2 participants