Approve engine refactor #260

flypaper0 · 2022-06-03T13:57:49Z

What changed

ApproveEngine created
NetworkInteractor callbacks replaced with publishers
Error handling for ApproveEngine

ApproveEngine works with events:

- proposeResponse(topic: String, proposal: SessionProposal)
- sessionProposal(Session.Proposal)
- sessionRejected

chadyj · 2022-06-03T17:21:59Z

Does this have a related issue?

Mike117687 · 2022-06-04T06:25:38Z

merge-these-changes

llbartekll · 2022-06-06T07:28:41Z

Sources/WalletConnectSign/Sign/Sign.swift

@@ -157,8 +157,8 @@ extension Sign {
    /// - Parameters:
    ///   - proposal: Session Proposal received from peer client in a WalletConnect delegate.
    ///   - reason: Reason why the session proposal was rejected. Conforms to CAIP25.
-    public func reject(proposal: Session.Proposal, reason: RejectionReason) {
-        client.reject(proposal: proposal, reason: reason)
+    public func reject(proposal: Session.Proposal, reason: RejectionReason) throws {


just a note: we are currently approving by proposalId(aka: proposer.publicKey) so maybe we should also rejecting by proposalId instead of requiring the whole Proposal object.

llbartekll · 2022-06-06T07:32:13Z

Sources/WalletConnectSign/Sign/SignClient.swift

-        guard let (sessionTopic, proposal) = pairingEngine.approveProposal(proposerPubKey: proposalId, validating: namespaces) else {return}
+        let (sessionTopic, proposal) = try approveEngine.approveProposal(proposerPubKey: proposalId, validating: namespaces)
        try sessionEngine.settle(topic: sessionTopic, proposal: proposal, namespaces: namespaces)


One of the motivations to have an approveEngine for me was to not pass (sessionTopic, proposal) between engines in the client.
a think the sessionEngine.settle method should also go to approveEngine
to me proposal response and settle request are like two pieces of an approval so make sense to perform them in one object.

llbartekll · 2022-06-06T07:34:08Z

Sources/WalletConnectSign/Sign/SignClient.swift

        try sessionEngine.settle(topic: sessionTopic, proposal: proposal, namespaces: namespaces)
    }

    /// For the responder to reject a session proposal.
    /// - Parameters:
    ///   - proposal: Session Proposal received from peer client in a WalletConnect delegate.
    ///   - reason: Reason why the session proposal was rejected. Conforms to CAIP25.
-    public func reject(proposal: Session.Proposal, reason: RejectionReason) {
-        pairingEngine.reject(proposal: proposal.proposal, reason: reason.internalRepresentation())
+    public func reject(proposal: Session.Proposal, reason: RejectionReason) throws {


can we also make it async method?
it could throw on networking errors then.

llbartekll · 2022-06-06T07:40:04Z

Sources/WalletConnectSign/Sign/SignClient.swift

+        approveEngine.approvePublisher.sink { [unowned self] response in
+            switch response {
+            case .proposeResponse(let topic, let proposal):
+                self.sessionEngine.settlingProposal = proposal
+                self.sessionEngine.setSubscription(topic: topic)
+            case .sessionProposal(let proposal):
+                self.delegate?.didReceive(sessionProposal: proposal)
+            case .sessionRejected(proposal: let proposal, reason: let reason):
+                self.delegate?.didReject(proposal: proposal, reason: reason.publicRepresentation())
+            }
+        }.store(in: &publishers)


Personally, I do not necessarily find enums publisher cleaner than callbacks 🤔
I think some people would also say that it is less Open–closed principle conformant

llbartekll · 2022-06-06T07:43:57Z

Sources/WalletConnectSign/Sign/SignClient.swift

@@ -157,16 +162,16 @@ public final class SignClient {
        namespaces: [String: SessionNamespace]


would be also nice if approve is async

flypaper0 · 2022-06-06T09:24:29Z

Does this have a related issue?

Before we doesn't handle some of approve errors which could occur. Now they handled and respond to another participant
Also I continue work, started by @llbartekll, of splitting client to Engines, to esolate responsabilities.

sekimondre · 2022-06-07T16:50:28Z

Sources/WalletConnectSign/Engine/Common/SessionEngine.swift

+    typealias SessionRequestCallback = (Request) -> Void
+    typealias SessionResponseCallback = (Response) -> Void
+    typealias SessionRejectedCallback = (String, SessionType.Reason) -> Void
+    typealias SessionDeleteCallback = (String, SessionType.Reason) -> Void
+    typealias EventReceivedCallback = (String, Session.Event, Blockchain?) -> Void


I'm not a fan of spreading too many typealiases through the code. They can make it harder to understand the code when used excessively.

sekimondre · 2022-06-07T16:52:01Z

Sources/WalletConnectSign/Engine/Controller/ApproveEngine.swift

+        // TODO: Delete pairing if inactive
+    }
+
+    func settle(topic: String, proposal: SessionProposal, namespaces: [String: SessionNamespace]) throws {


I wonder if something can make the settlement fail after the proposal is responded with a success, and if this can cause any problems. Something that we can think about in the future.

llbartekll · 2022-06-08T08:36:03Z

Sources/WalletConnectSign/Engine/Controller/ApproveEngine.swift

+
+    // MARK: SessionSettleRequest
+
+    func handleSessionSettleRequest(payload: WCRequestSubscriptionPayload, settleParams: SessionType.SettleParams) {


We moved ApproveEngine to /Controller dir but I see that you moved non-controller method handleSessionSettleRequest to ApproveEngine as well.

maybe we should later have /Non-Controller/SettleEngine and let it handle Session settle request?

Ok, Created TODO for that

* Cleanup Service + SequenceStore refactor (#241) * Cleanup service * Clean sessionToPairingTopic * SequenceStore refactor * Rename KeyValueStore -> CodableStore * Deliver an invite (#254) * Add Chat target, split packages * savepoint * Update networking interactor to decode unencrypted messages * pass on invite test * restructure chat's project folder * Add engine storages * extract storage domain identifiers * update logging * fix schemas * Update style * Add publishers to Chat * rename kv store to codable strore * UI tests (#242) * Pairing testcase * Ping testcase * ApproveSessionExistingPairing test case * Unused extensions deleted * Renamed to RegressionTests * UITests step on CI * CleanLaunch instead of deleting app * Fix test on Real device * Launch App fix * Approve engine refactor (#260) * Approve method moved ApproveEngine * Reject and wcSessionSubscriptions for ApproveEngine * Private methods moved in extension * ApproveEngine errors handlers * Try on reject * ApproveEngine moved to callbacks * Session Settle moved to approve Engine * onProposeResponse subscription removed * Reject by proposalId * Settle moved to approve * ApproveEngine moved to Controller folder * typealias removed * TODO for SettleEngine * #256 JSON-RPC Package (& Commons) (#261) * Add Chat target, split packages * savepoint * restructure chat's project folder * fix schemas * Add JSONRPC and Commons packages * Moved AnyCodable to Commons * Fixed test import * Reintroduces either type * Add request and response types * Add simple response decode tests * Add response ID parsing tests * Fixed tests typo * Improved response round trip coding test * Error response decoding tests * Invalid response decode tests * Enabled code coverage for library * Response decoding tests for structured result values * Add flexible initializers with tests * Add descriptions to errors thrown in response decoding * Renamed response internalResult to outcome * Basic RPC request decoding tests * Tests for request empty cases and corner cases * Add flexible inits for requests * Add identifier generation inits * Joined request notification extensions * Renamed files * Implemented default JSONRPC error cases * Declared RPCRequestConvertible as public * Remove rebase artifacts * Added debug description to request param primitives error Co-authored-by: Bartosz Rozwarski <bartus000@gmail.com> Co-authored-by: André Vants <MisterVants@users.noreply.github.com>

flypaper0 added 5 commits June 3, 2022 18:48

Approve method moved ApproveEngine

543c5ca

Reject and wcSessionSubscriptions for ApproveEngine

ed3f290

Private methods moved in extension

c8b3086

ApproveEngine errors handlers

2fc6791

Try on reject

d70ed7e

flypaper0 requested review from sekimondre and llbartekll June 3, 2022 13:57

flypaper0 changed the base branch from main to develop June 3, 2022 13:58

llbartekll reviewed Jun 6, 2022

View reviewed changes

flypaper0 added 6 commits June 6, 2022 18:03

ApproveEngine moved to callbacks

e4d8fef

Session Settle moved to approve Engine

8533da5

onProposeResponse subscription removed

3c3d9be

Reject by proposalId

4671a34

Settle moved to approve

16336ca

ApproveEngine moved to Controller folder

0195083

flypaper0 requested a review from llbartekll June 7, 2022 12:40

sekimondre approved these changes Jun 7, 2022

View reviewed changes

typealias removed

ba3fb76

llbartekll approved these changes Jun 8, 2022

View reviewed changes

TODO for SettleEngine

6bd3c6b

flypaper0 merged commit b3db843 into develop Jun 8, 2022

flypaper0 deleted the approve-engine-refactor branch June 8, 2022 10:58

flypaper0 mentioned this pull request Jun 9, 2022

Release v0.7.0-beta.101 #265

Merged

flypaper0 linked an issue Jun 16, 2022 that may be closed by this pull request

Refactor approval flow in engines #249

Closed

flypaper0 mentioned this pull request Jun 16, 2022

Refactor approval flow in engines #249

Closed

flypaper0 mentioned this pull request Jul 5, 2022

v0.8.0-beta.102 #308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Approve engine refactor #260

Approve engine refactor #260

flypaper0 commented Jun 3, 2022

chadyj commented Jun 3, 2022

Mike117687 commented Jun 4, 2022

llbartekll Jun 6, 2022

llbartekll Jun 6, 2022

llbartekll Jun 6, 2022

llbartekll Jun 6, 2022

llbartekll Jun 6, 2022

flypaper0 commented Jun 6, 2022

sekimondre Jun 7, 2022

sekimondre Jun 7, 2022

llbartekll Jun 8, 2022

flypaper0 Jun 8, 2022

		@@ -157,16 +162,16 @@ public final class SignClient {
		namespaces: [String: SessionNamespace]


		// MARK: SessionSettleRequest

		func handleSessionSettleRequest(payload: WCRequestSubscriptionPayload, settleParams: SessionType.SettleParams) {

Approve engine refactor #260

Approve engine refactor #260

Conversation

flypaper0 commented Jun 3, 2022

chadyj commented Jun 3, 2022

Mike117687 commented Jun 4, 2022

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

flypaper0 commented Jun 6, 2022

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