[Dispatching] Native socket support #911

alexander-lsvk · 2023-06-15T11:07:46Z

Due Dilligence

Description and motivation can be found here: https://www.notion.so/walletconnect/Native-Socket-Client-bdf981fa68f24a5e8ea4f285040a8e2c

Breaking change
Requires a documentation update

radeknovis

Code-wise looks good. I'm going to spend some time testing it.

flypaper0 · 2023-06-19T10:23:39Z

Sources/WalletConnectRelay/WebSocketClient/WebSocketClient.swift

+        let urlRequest = URLRequest(url: url)
+        socket = urlSession.webSocketTask(with: urlRequest)
+
+        isConnected = false


Why we're you're setting isConnected = false on reconnect?

yeah, strange

Because we are not actually connected yet but disconnected already. It will happen here

I am a bit concerned about handling the state of this flag properly, doesn't websocketTask or anything expose the state from urlsession itself?

flypaper0 · 2023-06-19T10:25:32Z

Sources/WalletConnectRelay/WebSocketClient/WebSocketClient.swift

+            }
+
+            if self.isConnected == true {
+                self.receiveMessage()


is it safe to call method from it self here? How it's working?

I also got confused, but apparently, this is by design from Apple. It needs to be called to receive further messages.

Radek is correct 😅 Agree this looks strange

llbartekll · 2023-06-19T17:11:45Z

Sources/WalletConnectRelay/WebSocketClient/WebSocketClient.swift

+    }
+}
+
+final class WebSocketClient: NSObject, WebSocketConnecting {


do we still need to conform to WebSocketConnecting protocol?
I was a starscream requirement
wouldn't it be better to refactor Dispatcher and socket would conform to this protocol?

I believe this protocol is more abstract in terms of testing. In case of conforming to URLSessionWebSocketTask we will need to depend on URLSessionWebSocketTask internal types like Message and CloseCode. But your suggestion is also valid, let's discuss on sync.

radeknovis · 2023-06-20T14:25:45Z

Sources/WalletConnectRelay/WebSocketClient/WebSocketClient.swift

+    var onText: ((String) -> Void)?
+    var request: URLRequest {
+        didSet {
+            if let url = request.url {


What is the goal of this didSet closure? I can see request is assigned only once in the init.

It's used for the updating socket url (new auth parameter) when automatic reconnect happens. It will be set here.

# Conflicts: # Example/ExampleApp.xcodeproj/project.pbxproj # Example/IntegrationTests/Auth/AuthTests.swift # Example/IntegrationTests/Chat/ChatTests.swift # Example/IntegrationTests/Sign/SignClientTests.swift # Example/IntegrationTests/Sync/SyncTests.swift # Sources/WalletConnectModal/Modal/Modal+Previews.swift # Sources/WalletConnectModal/Modal/ModalContainerView.swift # Sources/WalletConnectModal/Modal/ModalInteractor.swift # Sources/WalletConnectRelay/RelayClient.swift

arein

@alexander-lsvk the PR doesn't have a concise description with motivation, expected result, and how it was tested. Can you pelase add?

# Conflicts: # Example/ExampleApp.xcodeproj/project.pbxproj # Example/IntegrationTests/History/HistoryTests.swift # Example/IntegrationTests/Push/PushTests.swift # Example/IntegrationTests/Sync/SyncTests.swift # Example/RelayIntegrationTests/RelayClientEndToEndTests.swift # Example/WalletApp/ApplicationLayer/ConfigurationService.swift # Example/WalletApp/ApplicationLayer/Configurator/ThirdPartyConfigurator.swift # Sources/WalletConnectRelay/Dispatching.swift # Sources/WalletConnectRelay/RelayURLFactory.swift

# Conflicts: # Example/ExampleApp.xcodeproj/project.pbxproj # Example/WalletApp/ApplicationLayer/ConfigurationService.swift

simonmcl · 2023-11-06T15:13:31Z

Hi,

@llbartekll asked me to give some feedback here in response to #1205 . I was trying to use my own wrapper around URLSession as the websocket for this library and recently gave up due to some recurring issues I couldn't find the time to debug fully. My 2 cents on the issues and some of the comments in the notion document:

Relying on Third party libs:
I've always been a "don't use a lib unless necessary" type person, but the current project i'm working on really hammered that home. I spent months dealing with dozens of issues making my project impossible. E.g. I needed changes to the Swift version of the sodium crypto library, the creator used a cocoapods template project to set it up, which for opinionated reasons adds "Quick" and "Nimble" libraries for unit tests. The versions used were incompatible with the latest Xcode, causing all PRs to fail with no meaningful error displayed in github actions. The maintainers weren't really around and would only look at each PR to see if the build status failed. If it did they said they would not merge it and then disappear again. The painful part is that Quick and Nimble weren't actually used apart from a single unit test that cocoapods adds as an example, so all of this pain was completely unnecessary. After dozens of similar issues, i'm very cautious of using third party libraries. I've only been using starscream for 2 weeks now and already have 1 crash report from 1 of our internal testers from limited usage. The protocol supplied in WC2 matches an older version of starscream, I wasn't sure how some of the logic translated to the new one, so i've left it on 3.1.2 for now, and it feels very much like i'm back into the types of issues I had with sodium, caught between multiple versions
Using URLSession:
I found it quite easy to get it working in a basic setup, but it times out after 5 minutes of inactivity. I had a lot of trouble getting the reconnection flows to work in WC2 with this issue. Being unfamiliar with the business logic of the project adds some challenges to trying to figure this out. It would work well on simulator + device testing, but some combination of backgrounding / foregrounding / phone going into sleep etc, caused "something" to break a loop somewhere and the socket would just die until the app was killed. Wasn't easy to reproduce and haven't had the time to try investigate it in order to swap starscream back out

In my opinion the need for frequent reconnections is a design issue. I've previously done some work in non mobile spaces (limited web frontend, cloud, IoT, etc) that involved using sockets. Lots of devs love third party sockets like socket.io and starscream, as these custom implementations often don't adhere to the standard timeout rules in the websocket spec, and instead try to keep sockets open artificially longer or indefinitely in situations where it should have timed out. While from an initial developer experience point of view this is handy and easy to setup, it has very poor interoperability with other platforms + implementations. Every project i've worked on with a websocket has always implemented a custom keepalive on the application level. E.g. sending a "ping" every 60 seconds to keep the socket open. To me this is the first thing thats needed in WC2. Reconnections should be logic used for when the app is going to suspend / resume, not just a mechanism to keep the connection alive (outside of network issues obviously). Users will run into all sorts of issue with trying to send a payload to a device thats in the middle of a reconnection

While initially trying to debug this, I was trying to build a reconnect button + popups in my app to allow internal testers to try determine if the socket was down due to an issue connecting to WC server, or due to my handling of reconnections. I found the limitations around automatic versus manual network setup in WC2 to be very strange. I was confused why in automatic mode I was forbidden from calling a function to reset the socket and force it to connect again. I want the SDK to try manage all this for me, but don't see why, when an edge case arises, I can't tell it to manually disconnect + reconnect. I ended up switching to manual mode and handling the sleep / wake up process myself, so that I could add this button for testing. Managing it manually fixed some issues, as being unfamiliar with the logic of the SDK, it was easier and quicker to just clear everything and setup again. But it didn't fix everything (likely needing some kind of queue to manage multiple attempts to connect / reconnect coming in quickly)

I think custom keepalives and the ability to have a semi automatic network setup would go a long way to improving the usage of the library, and i'd be very keen to get rid of starscream and help test a custom implementation. Although my current app is not in production yet, we can run some experiments with beta testers after we get them on boarding in a couple of weeks

sonarcloud · 2024-02-26T15:09:17Z

Quality Gate passed

Issues
54 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

alexanderkhitev · 2024-04-03T13:47:23Z

hello the Wallet Connect team ! Any news/updates? Thanks.

llbartekll · 2024-04-04T16:04:45Z

hey @alexanderkhitev it's on hold right now

alexander-lsvk added 2 commits June 15, 2023 13:06

Add native WebSocket support

afce442

Remove test data

e49086b

alexander-lsvk temporarily deployed to internal June 15, 2023 11:07 — with GitHub Actions Inactive

Merge remote-tracking branch 'origin/develop' into native-socket

e6c609b

alexander-lsvk temporarily deployed to internal June 15, 2023 11:23 — with GitHub Actions Inactive

Fix merge issues

274438e

alexander-lsvk temporarily deployed to internal June 15, 2023 11:42 — with GitHub Actions Inactive

alexander-lsvk temporarily deployed to internal June 15, 2023 11:55 — with GitHub Actions Inactive

alexander-lsvk temporarily deployed to internal June 15, 2023 12:03 — with GitHub Actions Inactive

alexander-lsvk temporarily deployed to internal June 15, 2023 18:24 — with GitHub Actions Inactive

Merge branch 'develop' into native-socket

7f57858

alexander-lsvk temporarily deployed to internal June 16, 2023 08:48 — with GitHub Actions Inactive

alexander-lsvk requested review from radeknovis, llbartekll and flypaper0 June 16, 2023 09:27

radeknovis approved these changes Jun 16, 2023

View reviewed changes

flypaper0 reviewed Jun 19, 2023

View reviewed changes

llbartekll reviewed Jun 19, 2023

View reviewed changes

radeknovis reviewed Jun 20, 2023

View reviewed changes

alexander-lsvk requested review from flypaper0 and llbartekll July 2, 2023 14:14

alexander-lsvk temporarily deployed to internal July 2, 2023 14:32 — with GitHub Actions Inactive

alexander-lsvk added 2 commits July 2, 2023 17:16

Fix merge errors

70f2dd3

Fix merge errors

f9b74c8

alexander-lsvk temporarily deployed to internal July 2, 2023 15:17 — with GitHub Actions Inactive

flypaper0 approved these changes Jul 4, 2023

View reviewed changes

Update WebSocketConnecting protocol

10d8592

alexander-lsvk temporarily deployed to internal July 5, 2023 07:32 — with GitHub Actions Inactive

flypaper0 approved these changes Aug 11, 2023

View reviewed changes

asi90sp mentioned this pull request Aug 13, 2023

Block #1031

Closed

flypaper0 added the on hold label Aug 25, 2023

arein reviewed Aug 25, 2023

View reviewed changes

Support Starscream

6203975

alexander-lsvk temporarily deployed to internal September 7, 2023 06:01 — with GitHub Actions Inactive

alexander-lsvk temporarily deployed to internal September 7, 2023 06:08 — with GitHub Actions Inactive

Update socket client

1bc4842

alexander-lsvk temporarily deployed to internal September 12, 2023 11:34 — with GitHub Actions Inactive

Merge branch 'develop' into native-socket

2340bb3

alexander-lsvk temporarily deployed to internal September 12, 2023 11:44 — with GitHub Actions Inactive

Update tests

5642411

alexander-lsvk temporarily deployed to internal September 12, 2023 12:46 — with GitHub Actions Inactive

Fix tests

4bd0227

alexander-lsvk temporarily deployed to internal September 12, 2023 12:51 — with GitHub Actions Inactive

Update tests

748dc66

alexander-lsvk temporarily deployed to internal September 12, 2023 12:56 — with GitHub Actions Inactive

Update tests

366a891

alexander-lsvk temporarily deployed to internal September 14, 2023 07:15 — with GitHub Actions Inactive

alexander-lsvk temporarily deployed to internal September 14, 2023 07:23 — with GitHub Actions Inactive

Update

cf8168e

alexander-lsvk temporarily deployed to internal October 23, 2023 10:36 — with GitHub Actions Inactive

Merge branch 'develop' into native-socket

1ab4dbe

# Conflicts: # Example/ExampleApp.xcodeproj/project.pbxproj # Example/WalletApp/ApplicationLayer/ConfigurationService.swift

alexander-lsvk temporarily deployed to internal October 23, 2023 10:47 — with GitHub Actions Inactive

llbartekll mentioned this pull request Nov 6, 2023

Update WebSocketConnecting to match latest starscream #1205

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dispatching] Native socket support #911

[Dispatching] Native socket support #911

alexander-lsvk commented Jun 15, 2023 •

edited

Loading

radeknovis left a comment

flypaper0 Jun 19, 2023

llbartekll Jun 19, 2023

alexander-lsvk Jul 2, 2023 •

edited

Loading

llbartekll Jul 12, 2023

flypaper0 Jun 19, 2023

radeknovis Jun 20, 2023

alexander-lsvk Jul 2, 2023

llbartekll Jun 19, 2023

alexander-lsvk Jul 2, 2023

radeknovis Jun 20, 2023

alexander-lsvk Jul 2, 2023

arein left a comment

simonmcl commented Nov 6, 2023

sonarcloud bot commented Feb 26, 2024

alexanderkhitev commented Apr 3, 2024

llbartekll commented Apr 4, 2024

[Dispatching] Native socket support #911

Are you sure you want to change the base?

[Dispatching] Native socket support #911

Conversation

alexander-lsvk commented Jun 15, 2023 • edited Loading

Due Dilligence

radeknovis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-lsvk Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arein left a comment

Choose a reason for hiding this comment

simonmcl commented Nov 6, 2023

sonarcloud bot commented Feb 26, 2024

Quality Gate passed

alexanderkhitev commented Apr 3, 2024

llbartekll commented Apr 4, 2024

alexander-lsvk commented Jun 15, 2023 •

edited

Loading

alexander-lsvk Jul 2, 2023 •

edited

Loading