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

fix: relayer would only connect if there are topics present in the cl… #5654

Merged
merged 12 commits into from
Feb 20, 2025

Conversation

ganchoradkov
Copy link
Member

@ganchoradkov ganchoradkov commented Feb 18, 2025

Description

Implemented two changes based on https://www.notion.so/walletconnect/Core-SDK-Relay-reconnects-optimisation-1903a661771e80cebb47ccfae9fd905f

  • transportOpen will only open ws connection if there are topics for the client to subscribe
  • reconnection logic e.g. after ws drop, also checks for topics before reopening the connection, otherwise skips

Type of change

  • Chore (non-breaking change that addresses non-functional tasks, maintenance, or code quality improvements)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Draft PR (breaking/non-breaking change which needs more work for having a proper functionality [Mark this PR as ready to review only when completely ready])
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

tests

Checklist

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Additional Information (Optional)

Please include any additional information that may be useful for the reviewer.

@arein arein added the accepted label Feb 18, 2025
@@ -268,6 +272,13 @@ export class Relayer extends IRelayer {
}

async transportOpen(relayUrl?: string) {
if (!this.subscriber.hasAnyTopics) {
this.logger.warn(
"Starting WS connection skipped because the client has no topics to work with.",
Copy link
Member

Choose a reason for hiding this comment

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

should this be warn level or debug?

Copy link
Member Author

@ganchoradkov ganchoradkov Feb 18, 2025

Choose a reason for hiding this comment

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

it should be an error and the call should be rejected technically but we want to avoid breaking changes. Warn specifies something is not behaving as expected and I think it fits best. Debug will rarely be used by devs as its quite noisy

Copy link
Member

Choose a reason for hiding this comment

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

got it. So we don't want folks to interact with this API? Can we test on AppKit just to make sure we don't get loads of warnings now unexpectedly?

Copy link
Member

@arein arein left a comment

Choose a reason for hiding this comment

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

looks good - can you link me to the release process please? I suggest we test it in the lab

@ganchoradkov ganchoradkov marked this pull request as ready for review February 19, 2025 14:07
@ganchoradkov
Copy link
Member Author

looks good - can you link me to the release process please? I suggest we test it in the lab

the release checklist
https://github.com/WalletConnect/walletconnect-monorepo/blob/v2.0/.github/pr_template_release.md

Copy link
Member

@arein arein left a comment

Choose a reason for hiding this comment

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

LGTM let's make sure to do some thorough testing on AppKit and on the sample app

@ganchoradkov ganchoradkov merged commit 23cd671 into v2.0 Feb 20, 2025
10 checks passed
@ganchoradkov ganchoradkov deleted the fix/ws-connection-to-respect-topics branch February 20, 2025 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants