Update Starscream dependency to Major 4.x.x version.

[zulkis]

swift-nio-zlib-support subdependency removed.

Public WC interface was not affected.

[zulkis]

Just saw another 2 MRs :/

Will checkthem instead.

[zulkis]

Made a proof of work for TCPTransport custom WSEngine iOS 12.0+

Most likely we need to inject state from the outside, especially if we still want to guarantee iOS 11 support.

I, personally, do not see much sense in that iOS 11 support, better to spend time of MR to Starscream to make NativeEngine work as expected.

[zulkis]

It all works great until I try to connect with MetaMask. This thing is just something.
Right after MetaMask initiate connection I start to have
WSError(type: Starscream.ErrorType.securityError, message: "accept header doesn't match", code: 1)
Or POSIXErrorCode: Socket is not connected
Or POSIXErrorCode: Software caused connection abort
Or nw_read_request_report [C76] Receive failed with error "Socket is not connected"

Trust/Argent/Rainbow - work except Metamask.

[zulkis]

Looks like a problem in the new engine implementation.

If we pass WSEngine(transport: FoundationTransport(), certPinner: FoundationSecurity(), compressionHandler: nil) for engine parameter it all start working.

Basically it is fallback to the old Starscream implementation.

[DmitryBespalov]

Suggested change

	#if os(iOS)
	if let observer = self.backgroundNotificationObserver {
	NotificationCenter.default.removeObserver(observer)
	}
	if let observer = self.foregroundNotificationObserver {
	NotificationCenter.default.removeObserver(observer)
	}
	#endif

I think there's no need to call removeObserver in deinit (https://developer.apple.com/documentation/foundation/notificationcenter/1413994-removeobserver)

[zulkis]

Thanks for checking!

I actually see the opposite example in:
https://developer.apple.com/documentation/foundation/notificationcenter/1411723-addobserver

And it is logical in a sense that somebody who registers to notifications holds the reference and has control over it.
Am I missing something?

[DmitryBespalov]

I think we don't need to retain observers since the observation will exist for the duration of this class

Suggested change

	#if os(iOS)
	private var backgroundNotificationObserver: Any?
	private var foregroundNotificationObserver: Any?
	#endif

[zulkis]

Technically you're right, as blocks does not capture self, still, it will let our blocks passed to NotificationCenter be alive for an undefined amount of time. And contradicts the RAII principle, and leaves hanging memory in heap.

[zulkis]

https://developer.apple.com/documentation/foundation/notificationcenter/1411723-addobserver

The block that executes when receiving a notification.

The notification center copies the block. The notification center strongly holds the copied block until you remove the observer registration.

The block takes one argument: the notification.```

[zulkis]

@DmitryBespalov Is there anything that might need help to resolve this MR?

[DmitryBespalov]

@zulkis please check the other PR #84

I'll add the socket re-connection in the next pr

[oscahie]

It all works great until I try to connect with MetaMask. This thing is just something. Right after MetaMask initiate connection I start to have WSError(type: Starscream.ErrorType.securityError, message: "accept header doesn't match", code: 1) Or POSIXErrorCode: Socket is not connected Or POSIXErrorCode: Software caused connection abort Or nw_read_request_report [C76] Receive failed with error "Socket is not connected"

Trust/Argent/Rainbow - work except Metamask.

HI @zulkis apologies for resurfacing this old thread. I'm trying to reproduce the issues you mentioned above with Metamask (see this issue for context), and the only way I can is when using WalletConnectSwfit's example Client and the Metamask iOS app, both installed on the same iPhone. In this scenario I do see the socket disconnected errors, but AFAIK that's simply because either app will likely close/disconnect its sockets automatically once moved to background. If instead I use two different devices (or device for Metamask and simulator for Client) then it works fine, and also does with the native socket engine.

Can you confirm whether this is what you were seeing or was your use case something different perhaps?

[zulkis]

Hi @oscahie,

First of all - this pattern you've mentioned is one of the most important, in my opinion, as if my app and my wallet are on the same device - how else can I connect it?

I pretty much agree with everything what you've said in original proposal:
#121 (comment)

I believe Best solution would be to stop using StarScream altogether as it is full of known and visible bugs(Specifically bottom part of transport layer) and unsupported for quite a long time.

Even in that case I do not see how it will help resolving

POSIXErrorCode: Socket is not connected Or POSIXErrorCode: Software caused connection abort Or nw_read_request_report [C76] Receive failed with error "Socket is not connected"

Because this is low level error from OS itself and looks like that happens because of inner conflict between usage Metamask app(Which heavily uses WS) and client on the same device.

What I had as well experienced - is that I was losing whole network access on iPhone + MacOS(sic) when I have debugger attached to my app with WalletConnect library with Xcode and network as well was off on mac.
You can try this case as well with Client + Metamask App installed on Iphone connected to Xcode Debugger.

I could not fully remember all the details as my focus shifted last months but if you'd have specific questions - happy to help.

[oscahie]

Ok, that's a very fair point. I agree that both apps on the same device should be a supported scenario, and I have an idea to hopefully make it work with the native websockets engine too.

I'm working on it, will ping you again when I have something to show.