[Network] Refactor: #355 Relay client RPC #384

sekimondre · 2022-07-28T23:19:07Z

closes #355

Overview:

Integration of JSONRPC types into the Relay client target.
Introduces relay RPC methods declared conforming to RelayRPC protocol.
Small improvements to Either and RPCResponse.
ID generation logic for RPC methods decoupled in isolation.
Introduces improved and expanded RPCHistory to track all RPC requests sent and received from the client.

Notes:

RPCMethod protocol could be extracted to utils to be used in other targets for scalability.
RPCHistory is still only replacing the current behavior for Subscription method.

Next steps:

Expand RPCHistory usage to track all requests and responses, from local and remote origins, to provide a full history.
Implement RPC responding for parsing and other errors on client side.
Find a solution to avoid leaking cancellables on error calls.

llbartekll · 2022-07-29T06:42:51Z

Sources/WalletConnectRelay/RelayClient.swift

-        dispatcher.send(requestJson) { [weak self] error in
+        cancellable = requestAcknowledgePublisher
+            .filter { $0 == request.id }
+            .sink { (_) in
+            cancellable?.cancel()
+                onNetworkAcknowledge(nil)
+        }
+        dispatcher.send(message) { [weak self] error in
            if let error = error {
                self?.logger.debug("Failed to Publish Payload, error: \(error)")
                cancellable?.cancel()
                onNetworkAcknowledge(error)
            }
        }
-        cancellable = requestAcknowledgePublisher
-            .filter {$0.id == request.id}
-            .sink { (_) in
-            cancellable?.cancel()
-                onNetworkAcknowledge(nil)
-        }


any advantage of changing the order?

Setting up the events before the call prevents any race conditions in case of local socket errors. It's just a good practice

Socket error will be handled in the completion block on a different thread so I think it's not the case.
Subscription will happen right after send() method execution as its just adds execution block.
correct me if I am wrong but imo the risk here is close to 0

if send would be a throwing function, error could happen before subscription but if error happens, you do not even care about subscription. anyway it does not apply for above code.

anyway, you can leave it like this, won't argue 😄

merhaba birşey sormak istiyorum

llbartekll · 2022-07-29T06:57:31Z

Sources/WalletConnectUtils/RPCHistory.swift

@@ -0,0 +1,67 @@
+import JSONRPC
+
+public final class RPCHistory {


I wonder how new history can affect existing integrations 🤔
any thoughts?

Currently, the history is being used just as a means of message deduplication, so the worst case scenario risk is the SDK receiving a duplicate Subscription once, which looks like a small trade-off to migrate to a history that can track all requests (the current implementation is generic, so it's bound to a single type of params). What do you think?

Also, we still have the transition to version 1.0.0 in the future as an opportunity to include all breaking changes that we need.

I think we want to avoid any breaking changes that could affect apps on production. But currently I do not think it will be very dangerous. Anyway it's worth to test it before merging.

llbartekll

Looks good! wish to see a generic networking interactor :)

llbartekll · 2022-08-01T08:57:27Z

Sources/WalletConnectRelay/RPC/RelayRPC.swift

+    func wrapToIridium() -> PrefixDecorator<Self> {
+        return PrefixDecorator(rpcMethod: self, prefix: "iridium")
+    }
+
+    func wrapToIRN() -> PrefixDecorator<Self> {
+        return PrefixDecorator(rpcMethod: self, prefix: "irn")
+    }


As I mentioned before I do not like those wrappings.
I'd prefer concrete types like IrnPublish etc.
imho, it adds extra unnecessary complexity, couples RelayRPC to wrapToIridium/wrapToIRN and eventually it will leave some wrapping methods that are not in use.

We already saw the the protocol be renamed twice, it's better to be prepared for any future changes. Ideally, the RelayClient calls should be refactored in a way that the wrapping is called in just one line, so it's easy to change it.

I think we need to have a way to configure this in single place (not for every request).

After next protocol renaming is better to replace method than add another wrapToXXX method 😁

llbartekll · 2022-08-01T09:48:36Z

Sources/WalletConnectRelay/RelayClient.swift

        guard let subscriptionId = subscriptions[topic] else {
-            completion(RelyerError.subscriptionIdNotFound)
-            return nil
+            completion(RelayerError.subscriptionIdNotFound)


Maybe a good time to rename the error. We no longer use Relayer anywhere. Errors would be consistent with other classes.

llbartekll · 2022-08-01T10:04:34Z

Sources/WalletConnectRelay/RelayClient.swift

+                    subscriptionResponsePublisherSubject.send((response.id, subscriptionId))
+                }
+            case .failure(let rpcError):
+                logger.error("Received error message from iridium network, code: \(rpcError.code), message: \(rpcError.message)")


maybe we could use relay network instead of iridium network? as it is suppose to change.

True, I've removed references to the "iridium" naming

llbartekll · 2022-08-01T10:08:03Z

Sources/WalletConnectSign/NetworkInteractor/NetworkRelaying.swift

-    @discardableResult func publish(topic: String, payload: String, tag: Int, prompt: Bool, onNetworkAcknowledge: @escaping ((Error?) -> Void)) -> Int64
+    func publish(topic: String, payload: String, tag: Int, prompt: Bool, onNetworkAcknowledge: @escaping ((Error?) -> Void))


👍 yeah that was really not necessary to return an id.

flypaper0 · 2022-08-01T14:03:05Z

Package.swift

@@ -36,7 +36,7 @@ let package = Package(
            path: "Sources/Auth"),
        .target(
            name: "WalletConnectRelay",
-            dependencies: ["WalletConnectUtils", "WalletConnectKMS"],


WalletConnectUtils depends on JSONRPC, do we need to add JSONRPC dependency here also?

flypaper0 · 2022-08-01T14:03:23Z

Package.swift

            path: "Tests/TestingUtils"),
        .testTarget(
            name: "WalletConnectUtilsTests",
-            dependencies: ["WalletConnectUtils"]),


Sources/Commons/Either.swift

flypaper0 · 2022-08-01T14:15:53Z

Sources/WalletConnectRelay/RelayClient.swift

-        let requestJson = try request.json()
-        try await dispatcher.send(requestJson)
+        let request = Publish(params: .init(topic: topic, message: payload, ttl: defaultTtl, prompt: prompt, tag: tag))
+            .wrapToIridium()


It's easy to miss this wrap on new request implementation. We need to configure it once IG

In one of my first experiments, the wrap + transform into RPC needed to be called only in a single place before sending the message, but I couldn't fit it in the current implementation (because of the callback problems that I was trying to solve). I agree that it should be called just once.

flypaper0 · 2022-08-01T14:44:01Z

Sources/WalletConnectRelay/RelayClient.swift

        var cancellable: AnyCancellable?
-        dispatcher.send(requestJson) { [weak self] error in
+        cancellable = subscriptionResponsePublisher


is it possible to replace completion with withCheckedThrowingContinuation? To make this method async?

The problem is that we need to wait for the acknowledgement with the subscription ID, the same problem that I was talking about in out sync meeting, that I couldn't solve yet

flypaper0 · 2022-08-01T14:45:00Z

Sources/WalletConnectRelay/RelayClient.swift

        var cancellable: AnyCancellable?
-        dispatcher.send(requestJson) { [weak self] error in
+        cancellable = requestAcknowledgePublisher


Need to find way how to move requestAcknowledgePublisher subscription out of this method.

Yes, the RelayClient refactor is not over

llbartekll · 2022-08-02T07:37:17Z

Sources/WalletConnectUtils/RPCHistory.swift

+        }
+        let id: RPCID
+        let topic: String
+        let origin: Origin


just noticed it. What is the purpose of origin?

Being able to track whether a request was sent from the client or from the peer. Thinking on long term support, I think is a nice thing to have, might be useful in the future

llbartekll · 2022-08-02T07:41:10Z

Sources/WalletConnectRelay/RelayClient.swift

+        if let request = tryDecode(RPCRequest.self, from: payload) {
+            if let params = try? request.params?.get(Subscription.Params.self) {
+                do {
+                    try rpcHistory.set(request, forTopic: params.data.topic, emmitedBy: .remote)


it is the only place where we are setting a history record. Should we record history when requesting as well?

Right now, it's the only place. I'm in for recording all requests, so we can have a complete history.

llbartekll reviewed Jul 29, 2022

View reviewed changes

sekimondre requested review from flypaper0 and llbartekll July 31, 2022 23:08

sekimondre self-assigned this Jul 31, 2022

sekimondre added the enhancement New feature or request label Jul 31, 2022

sekimondre marked this pull request as ready for review July 31, 2022 23:09

llbartekll reviewed Aug 1, 2022

View reviewed changes

André Vants added 9 commits August 1, 2022 10:16

Successful integration on publish test

043c1ce

Replaced unsubscribe call and fixed test

caf3809

Replaced subscribe call and added test

9e86741

Add subscription parsing

d8d8564

Introduces new and improved RPC history

84fb09d

Replace rpc history used in relay client

ccacb2f

Removed old relay method types

527fb66

Organize files and folders

4c6a553

Add unit tests for RPCHistory

156e32b

sekimondre force-pushed the refactor/#355-relay-rpc branch from 4fd874a to 156e32b Compare August 1, 2022 13:16

André Vants added 2 commits August 1, 2022 10:50

Removed some references of iridium naming in relay

ad826a6

Renamed relayer errors

a9fc882

flypaper0 reviewed Aug 1, 2022

View reviewed changes

Sources/Commons/Either.swift Show resolved Hide resolved

flypaper0 reviewed Aug 1, 2022

View reviewed changes

Removed unnecessary dependencies

ccd153f

flypaper0 reviewed Aug 1, 2022

View reviewed changes

Fixed broken keychain test

9b7f82f

flypaper0 approved these changes Aug 1, 2022

View reviewed changes

sekimondre merged commit 21b6598 into develop Aug 1, 2022

sekimondre deleted the refactor/#355-relay-rpc branch August 1, 2022 15:29

llbartekll reviewed Aug 2, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Network] Refactor: #355 Relay client RPC #384

[Network] Refactor: #355 Relay client RPC #384

sekimondre commented Jul 28, 2022 •

edited

Loading

llbartekll Jul 29, 2022

sekimondre Jul 29, 2022

llbartekll Jul 29, 2022

llbartekll Jul 29, 2022

jjgikufg Aug 1, 2022

llbartekll Jul 29, 2022

sekimondre Jul 29, 2022

sekimondre Jul 29, 2022

llbartekll Jul 29, 2022

llbartekll left a comment

llbartekll Aug 1, 2022

sekimondre Aug 1, 2022

flypaper0 Aug 1, 2022

llbartekll Aug 1, 2022

sekimondre Aug 1, 2022

llbartekll Aug 1, 2022

sekimondre Aug 1, 2022

llbartekll Aug 1, 2022

flypaper0 Aug 1, 2022

sekimondre Aug 1, 2022

flypaper0 Aug 1, 2022

sekimondre Aug 1, 2022

flypaper0 Aug 1, 2022

sekimondre Aug 1, 2022

flypaper0 Aug 1, 2022

sekimondre Aug 1, 2022

flypaper0 Aug 1, 2022

sekimondre Aug 1, 2022

llbartekll Aug 2, 2022

sekimondre Aug 4, 2022

llbartekll Aug 2, 2022

sekimondre Aug 4, 2022

		@@ -0,0 +1,67 @@
		import JSONRPC

		public final class RPCHistory {

		@discardableResult func publish(topic: String, payload: String, tag: Int, prompt: Bool, onNetworkAcknowledge: @escaping ((Error?) -> Void)) -> Int64
		func publish(topic: String, payload: String, tag: Int, prompt: Bool, onNetworkAcknowledge: @escaping ((Error?) -> Void))

[Network] Refactor: #355 Relay client RPC #384

[Network] Refactor: #355 Relay client RPC #384

Conversation

sekimondre commented Jul 28, 2022 • edited Loading

Overview:

Notes:

Next steps:

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

llbartekll left a 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

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

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

sekimondre commented Jul 28, 2022 •

edited

Loading