Remove Starscream as dependency, use Apple's URLSession-based websockets instead #124
Conversation
…using Apple's `URLSessionWebSocketTask` for the websockets connection, which means iOS 13.0+ is required.
…e's no connectivity
| return URLSession(configuration: configuration, delegate: delegate, delegateQueue: nil) | ||
| }() | ||
|
|
||
| #if os(iOS) |
There was a problem hiding this comment.
I'm using #if os(iOS) just because I saw it in #81 but I wonder, is it really necessary? Doesn't look like other platforms are explicitly supported (beyond M1 macs running iOS apps)?
There was a problem hiding this comment.
Package's platform include macOS and the UIBackgroundTaskIdentifier does not exist on macOS, so I thing we're fine here
|
Note that one thing that is not supported in this implementation is responding to incoming |
…t into oscar/drop-starscream
|
@oscahie ok, so we're still testing the changes. I found some errors when trying to integrate to our main projects and this is the branch with fixes which is based on your changes and on other library tweaks we made before: https://github.com/gnosis/WalletConnectSwift/tree/oscar/drop-starscream I believe these changes are good for a next minor release, I'll bump the version number. Also, do you want to be mentioned in the Contributors section in the Readme? |
|
Great work 💪 Would it be possible to update CryptoSwift to the latest version? (maybe in another PR?) |
|
@DmitryBespalov cool, please give it as much testing as you need until you feel comfortable with merging the changes. It's also nice to see you've been making some improvements in that gnosis fork, specially for the session approval delegate method, which is the thing I was just implementing today in our wallet app, and indeed, the current API is quite cumbersome to work with when asynchronous user approval is involved. It'd just have been awesome though if you had decided to move away entirely from using the delegation pattern and instead use closure callbacks, IMHO :) It's cool if you want to add me to the contributors, though I can't promise that I'll be contributing regularly. It just so happens that the project I'm working on at the moment needs to use this library, and so I've been tasked with the integration and making the necessary changes for it to succeed. In the hopefully-not-so-distant future we expect to move to v2 of the protocol though. |
|
@acecilia OK! @oscahie yes, my view of the current version of the library is that it is hard to use and its domain model is awkward. For going forward I have a vision for making the library modular and adding other companion libraries which would handle persistence of connections, and a standard UI with the list of wallets or dapps, as well as handling of requests/responses. Ideally, this would be all-in-one package that wallet and dapp developers integrate in their app, because I think that additional use cases around wallet-connect are repeated over and over for each project. Then, the v2 of the protocol will also be added, and the library would gracefully switch between the protocols based on the protocol version in the link. This way, if it's easy to integrate into other projects and with v2/v1 handling in the same library, we can advance the transition to the v2 across wallets and dapps on iOS. |
|
@oscahie regarding PONG messages from the client to the channel
As far as the specs go about the ping/pong https://datatracker.ietf.org/doc/html/rfc6455#section-5.5.2 and based on apple's documentation https://developer.apple.com/documentation/foundation/urlsessionwebsockettask/3181206-sendping there's no way to send a pong with Apple's API. My assumption is as yours, that the framework handles the PONG frames received automatically. |
… reliability of the connection in general and have the socket reconnected quicker when needed.
|
Added a small improvement today that treats any errors while sending data or pings as connection errors, which should help to detect failing connections quicker. In fact I had a rare case today in which I disabled all connectivity in my phone and Apple's |
|
@oscahie we completed testing of the fork I shared before with your changes. To the best of our knowledge, everything still works well. To be able to merge, your branch needs to be updated with the changes in the gnosis fork. I'll create a merge request to your repository first, and after merging that, we'll be able to merge this one. Then, in another PR, I'll also update a CryptoSwift library and double check the package manager configurations, and then release new version. |
Fixes and improvements
|
Cool, I merged that one. Also would be great if you could take a look at #125 I just opened, hopefully can be included in the next release too. |
This PR removes Starscream as dependency in favour of using Apple's
URLSessionWebSocketTaskdirectly in theWebSocketConnectionclass (see motivation).The minimum required iOS version has been updated to iOS 13.0 due
URLSessionWebSocketTaskhaving been introduced on iOS 13.I did quite a bit of testing trying to cover various edge cases related to connectivity issues, such as attempting to initiate a new session with no network availability, or having the app lose the connection temporarily. Since it uses
URLSession's built-inwaitsForConnectivityconfiguration option, I think it now behaves significantly better than what it was before (retrying indefinitely at a very rapid pace, using lots of CPU and memory).The disconnect detection issues reported in 1.6.2 do not occur with these changes.
Finally, it also addresses the issue mentioned here that happened with Startscream's native engine (a light wrapper around
URLSessionWebSocketTask). The solution revolves around requesting additional background execution time from the OS whenever the app moves to the background, so that the socket can continue working enough time for the wallet peer to accept or reject the session. I tested it particularly with the Metamask app and it worked great.