From 803a300d24d0e076490f5baa1f4041d0aaebef7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Vants?= Date: Fri, 28 Jan 2022 18:50:45 -0300 Subject: [PATCH 1/6] Add permission checks to upgrade call and success test --- .../WalletConnect/Engine/SessionEngine.swift | 33 +++++++++++++++++-- Sources/WalletConnect/Types/ReasonCode.swift | 5 +++ .../WalletConnect/WalletConnectClient.swift | 4 +-- Tests/IntegrationTests/ClientTest.swift | 4 +-- .../SessionEngineTests.swift | 26 +++++++++++++++ Tests/WalletConnectTests/Stub/Stubs.swift | 29 +++++++++++++--- 6 files changed, 89 insertions(+), 12 deletions(-) diff --git a/Sources/WalletConnect/Engine/SessionEngine.swift b/Sources/WalletConnect/Engine/SessionEngine.swift index 453ab0cd4..39753b494 100644 --- a/Sources/WalletConnect/Engine/SessionEngine.swift +++ b/Sources/WalletConnect/Engine/SessionEngine.swift @@ -213,10 +213,18 @@ final class SessionEngine { relayer.request(.wcSessionUpdate(SessionType.UpdateParams(accounts: accounts)), onTopic: topic) } - func upgrade(topic: String, permissions: Session.Permissions) { + func upgrade(topic: String, permissions: Session.Permissions) throws { guard var session = try? sequencesStore.getSequence(forTopic: topic) else { - logger.debug("Could not find session for topic \(topic)") - return + fatalError() + } + guard session.isSettled else { + fatalError() + } + guard isController else { + fatalError() + } + guard validatePermissions(permissions) else { + fatalError() } session.upgrade(permissions) guard let newPermissions = session.settled?.permissions else { @@ -234,6 +242,25 @@ final class SessionEngine { } } + private func validatePermissions(_ permissions: Session.Permissions) -> Bool { + for chainId in permissions.blockchains { + if !String.conformsToCAIP2(chainId) { + return false + } + } + for method in permissions.methods { + if method.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { + return false + } + } + for notification in permissions.notifications { + if notification.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { + return false + } + } + return true + } + func notify(topic: String, params: Session.Notification, completion: ((Error?)->())?) { guard let session = try? sequencesStore.getSequence(forTopic: topic), session.isSettled else { logger.debug("Could not find session for topic \(topic)") diff --git a/Sources/WalletConnect/Types/ReasonCode.swift b/Sources/WalletConnect/Types/ReasonCode.swift index f253fe07d..cfcbd03f3 100644 --- a/Sources/WalletConnect/Types/ReasonCode.swift +++ b/Sources/WalletConnect/Types/ReasonCode.swift @@ -5,9 +5,14 @@ enum ReasonCode { case session = "session" } + // 0 (Generic) case generic(message: String) + + // 1000 (Internal) case invalidUpdateRequest(context: Context) case noContextWithTopic(context: Context, topic: String) + + // 3000 (Unauthorized) case unauthorizedUpdateRequest(context: Context) case unauthorizedMatchingController(isController: Bool) diff --git a/Sources/WalletConnect/WalletConnectClient.swift b/Sources/WalletConnect/WalletConnectClient.swift index 016617d5e..6dccf8ee4 100644 --- a/Sources/WalletConnect/WalletConnectClient.swift +++ b/Sources/WalletConnect/WalletConnectClient.swift @@ -148,8 +148,8 @@ public final class WalletConnectClient { /// - Parameters: /// - topic: Topic of the session that is intended to be upgraded. /// - permissions: Sets of permissions that will be combined with existing ones. - public func upgrade(topic: String, permissions: Session.Permissions) { - sessionEngine.upgrade(topic: topic, permissions: permissions) + public func upgrade(topic: String, permissions: Session.Permissions) throws { + try sessionEngine.upgrade(topic: topic, permissions: permissions) } /// For the proposer to send JSON-RPC requests to responding peer. diff --git a/Tests/IntegrationTests/ClientTest.swift b/Tests/IntegrationTests/ClientTest.swift index cb56eff3c..e9a5e12f2 100644 --- a/Tests/IntegrationTests/ClientTest.swift +++ b/Tests/IntegrationTests/ClientTest.swift @@ -251,7 +251,7 @@ final class ClientTests: XCTestCase { self.responder.client.approve(proposal: proposal, accounts: [account]) } responder.onSessionSettled = { [unowned self] sessionSettled in - responder.client.upgrade(topic: sessionSettled.topic, permissions: upgradePermissions) + try? responder.client.upgrade(topic: sessionSettled.topic, permissions: upgradePermissions) } proposer.onSessionUpgrade = { topic, permissions in XCTAssertTrue(permissions.blockchains.isSuperset(of: upgradePermissions.blockchains)) @@ -280,7 +280,7 @@ final class ClientTests: XCTestCase { self.responder.client.approve(proposal: proposal, accounts: [account]) } proposer.onSessionSettled = { [unowned self] sessionSettled in - proposer.client.upgrade(topic: sessionSettled.topic, permissions: upgradePermissions) + try? proposer.client.upgrade(topic: sessionSettled.topic, permissions: upgradePermissions) } proposer.onSessionUpgrade = { topic, permissions in proposerSessionUpgradeExpectation.fulfill() diff --git a/Tests/WalletConnectTests/SessionEngineTests.swift b/Tests/WalletConnectTests/SessionEngineTests.swift index 249e9c8a8..4249785ad 100644 --- a/Tests/WalletConnectTests/SessionEngineTests.swift +++ b/Tests/WalletConnectTests/SessionEngineTests.swift @@ -349,4 +349,30 @@ final class SessionEngineTests: XCTestCase { } // TODO: Update acknowledgement tests + + // MARK: - Upgrade call tests + + func testUpgradeSuccess() throws { + setupEngine(isController: true) + let session = SessionSequence.stubSettled() + storageMock.setSequence(session) + try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub()) + XCTAssertTrue(relayMock.didCallRequest) + } + + func testUpgradeErrorSessionNotFound() { + + } + + func testUpgradeErrorSessionNotSettled() { + + } + + func testUpgradeErrorInvalidPermissions() { + + } + + func testUpgradeErrorCalledByNonController() { + + } } diff --git a/Tests/WalletConnectTests/Stub/Stubs.swift b/Tests/WalletConnectTests/Stub/Stubs.swift index d4d281ad3..a3adf8321 100644 --- a/Tests/WalletConnectTests/Stub/Stubs.swift +++ b/Tests/WalletConnectTests/Stub/Stubs.swift @@ -17,12 +17,31 @@ extension Pairing { } } +extension Session.Permissions { + static func stub( + chains: Set = ["solana:4sGjMW1sUnHzSxGspuhpqLDx6wiyjNtZ"], + methods: Set = ["getGenesisHash"], + notifications: [String] = ["msg"] + ) -> Session.Permissions { + Session.Permissions( + blockchains: chains, + methods: methods, + notifications: notifications + ) + } +} + extension SessionPermissions { - static func stub(controllerKey: String = AgreementPrivateKey().publicKey.hexRepresentation) -> SessionPermissions { - SessionPermissions( - blockchain: Blockchain(chains: []), - jsonrpc: JSONRPC(methods: []), - notifications: Notifications(types: []), + static func stub( + chains: Set = ["eip155:1"], + jsonrpc: Set = ["eth_sign"], + notifications: [String] = ["a_type"], + controllerKey: String = AgreementPrivateKey().publicKey.hexRepresentation + ) -> SessionPermissions { + return SessionPermissions( + blockchain: Blockchain(chains: chains), + jsonrpc: JSONRPC(methods: jsonrpc), + notifications: Notifications(types: notifications), controller: Controller(publicKey: controllerKey) ) } From 4ecf35686f4ce75bfc11fa8bf51cf37ba7266583 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Vants?= Date: Fri, 28 Jan 2022 19:18:25 -0300 Subject: [PATCH 2/6] Unit tests for session upgrade error cases --- .../WalletConnect/Engine/SessionEngine.swift | 8 ++--- .../WalletConnect/WalletConnectError.swift | 17 ++++++---- .../Helpers/Error+Extension.swift | 27 ++++++++++++++++ .../SessionEngineTests.swift | 32 ++++++++++++++++--- 4 files changed, 69 insertions(+), 15 deletions(-) diff --git a/Sources/WalletConnect/Engine/SessionEngine.swift b/Sources/WalletConnect/Engine/SessionEngine.swift index 2d2089937..cc0bd220f 100644 --- a/Sources/WalletConnect/Engine/SessionEngine.swift +++ b/Sources/WalletConnect/Engine/SessionEngine.swift @@ -217,16 +217,16 @@ final class SessionEngine { func upgrade(topic: String, permissions: Session.Permissions) throws { guard var session = sequencesStore.getSequence(forTopic: topic) else { - fatalError() + throw WalletConnectError.noSessionMatchingTopic(topic) } guard session.isSettled else { - fatalError() + throw WalletConnectError.sessionNotSettled } guard isController else { - fatalError() + throw WalletConnectError.unauthorizedNonControllerCall } guard validatePermissions(permissions) else { - fatalError() + throw WalletConnectError.invalidPermissions } session.upgrade(permissions) guard let newPermissions = session.settled?.permissions else { diff --git a/Sources/WalletConnect/WalletConnectError.swift b/Sources/WalletConnect/WalletConnectError.swift index a05eb5a06..5d28c49dc 100644 --- a/Sources/WalletConnect/WalletConnectError.swift +++ b/Sources/WalletConnect/WalletConnectError.swift @@ -2,19 +2,18 @@ import Foundation +// TODO: Migrate protocol errors to ReasonCode enum over time. Use WalletConnectError for client errors only. enum WalletConnectError: Error { - // 1000 (Internal) + case noSessionMatchingTopic(String) + case sessionNotSettled + case invalidPermissions + case unauthorizedNonControllerCall + case `internal`(_ reason: InternalReason) - // 2000 (Timeout) - // 3000 (Unauthorized) case unauthrorized(_ reason: UnauthorizedReason) - // 4000 (EIP-1193) - // 5000 (CAIP-25) - // 9000 (Unknown) - enum InternalReason: Error { case notApproved case malformedPairingURI @@ -50,6 +49,8 @@ extension WalletConnectError: CustomStringConvertible { return reason.code case .unauthrorized(let reason): return reason.code + default: + return 0 } } @@ -59,6 +60,8 @@ extension WalletConnectError: CustomStringConvertible { return reason.description case .unauthrorized(let reason): return reason.description + default: + return "" } } } diff --git a/Tests/WalletConnectTests/Helpers/Error+Extension.swift b/Tests/WalletConnectTests/Helpers/Error+Extension.swift index d21783353..25845a8cb 100644 --- a/Tests/WalletConnectTests/Helpers/Error+Extension.swift +++ b/Tests/WalletConnectTests/Helpers/Error+Extension.swift @@ -7,3 +7,30 @@ extension NSError { NSError(domain: "com.walletconnect.sdk.tests.error", code: code, userInfo: nil) } } + +extension Error { + + var wcError: WalletConnectError? { + self as? WalletConnectError + } + + var isNoSessionMatchingTopicError: Bool { + guard case .noSessionMatchingTopic = wcError else { return false } + return true + } + + var isSessionNotSettledError: Bool { + guard case .sessionNotSettled = wcError else { return false } + return true + } + + var isInvalidPermissionsError: Bool { + guard case .invalidPermissions = wcError else { return false } + return true + } + + var isUnauthorizedNonControllerCallError: Bool { + guard case .unauthorizedNonControllerCall = wcError else { return false } + return true + } +} diff --git a/Tests/WalletConnectTests/SessionEngineTests.swift b/Tests/WalletConnectTests/SessionEngineTests.swift index 39e5ffe0d..e2e11eb6d 100644 --- a/Tests/WalletConnectTests/SessionEngineTests.swift +++ b/Tests/WalletConnectTests/SessionEngineTests.swift @@ -361,18 +361,42 @@ final class SessionEngineTests: XCTestCase { } func testUpgradeErrorSessionNotFound() { - + setupEngine(isController: true) + XCTAssertThrowsError(try engine.upgrade(topic: "", permissions: Session.Permissions.stub())) { error in + XCTAssertTrue(error.isNoSessionMatchingTopicError) + } } func testUpgradeErrorSessionNotSettled() { - + setupEngine(isController: true) + let session = SessionSequence.stubPreSettled() + storageMock.setSequence(session) + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub())) { error in + XCTAssertTrue(error.isSessionNotSettledError) + } } func testUpgradeErrorInvalidPermissions() { - + setupEngine(isController: true) + let session = SessionSequence.stubSettled() + storageMock.setSequence(session) + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub(chains: [""]))) { error in + XCTAssertTrue(error.isInvalidPermissionsError) + } + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub(methods: [""]))) { error in + XCTAssertTrue(error.isInvalidPermissionsError) + } + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub(notifications: [""]))) { error in + XCTAssertTrue(error.isInvalidPermissionsError) + } } func testUpgradeErrorCalledByNonController() { - + setupEngine(isController: false) + let session = SessionSequence.stubSettled() + storageMock.setSequence(session) + XCTAssertThrowsError(try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub())) { error in + XCTAssertTrue(error.isUnauthorizedNonControllerCallError) + } } } From 2400c3506fd624830634d1add5fafc8368c3a606 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Vants?= Date: Fri, 28 Jan 2022 19:36:27 -0300 Subject: [PATCH 3/6] Removed response completion block from upgrade request --- .../WalletConnect/Engine/SessionEngine.swift | 30 +++++++++++-------- .../SessionEngineTests.swift | 4 ++- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/Sources/WalletConnect/Engine/SessionEngine.swift b/Sources/WalletConnect/Engine/SessionEngine.swift index cc0bd220f..3fd54690f 100644 --- a/Sources/WalletConnect/Engine/SessionEngine.swift +++ b/Sources/WalletConnect/Engine/SessionEngine.swift @@ -229,19 +229,9 @@ final class SessionEngine { throw WalletConnectError.invalidPermissions } session.upgrade(permissions) - guard let newPermissions = session.settled?.permissions else { - return - } - relayer.request(.wcSessionUpgrade(SessionType.UpgradeParams(permissions: newPermissions)), onTopic: topic) { [unowned self] result in - switch result { - case .success(_): - sequencesStore.setSequence(session) - onSessionUpgrade?(session.topic, newPermissions) - case .failure(_): - return - //TODO - } - } + let newPermissions = session.settled!.permissions // We know session is settled + sequencesStore.setSequence(session) + relayer.request(.wcSessionUpgrade(SessionType.UpgradeParams(permissions: newPermissions)), onTopic: topic) } private func validatePermissions(_ permissions: Session.Permissions) -> Bool { @@ -542,6 +532,8 @@ final class SessionEngine { handleApproveResponse(topic: response.topic, result: response.result) case .sessionUpdate: handleUpdateResponse(topic: response.topic, result: response.result) + case .sessionUpgrade: + handleUpgradeResponse(topic: response.topic, result: response.result) default: break } @@ -601,4 +593,16 @@ final class SessionEngine { logger.error("Peer failed to update state.") } } + + private func handleUpgradeResponse(topic: String, result: Result, Error>) { + guard let session = sequencesStore.getSequence(forTopic: topic), let permissions = session.settled?.permissions else { + return + } + switch result { + case .success: + onSessionUpgrade?(session.topic, permissions) + case .failure: + logger.error("Peer failed to upgrade permissions.") + } + } } diff --git a/Tests/WalletConnectTests/SessionEngineTests.swift b/Tests/WalletConnectTests/SessionEngineTests.swift index e2e11eb6d..c97b2ea0b 100644 --- a/Tests/WalletConnectTests/SessionEngineTests.swift +++ b/Tests/WalletConnectTests/SessionEngineTests.swift @@ -354,10 +354,12 @@ final class SessionEngineTests: XCTestCase { func testUpgradeSuccess() throws { setupEngine(isController: true) + let permissions = Session.Permissions.stub() let session = SessionSequence.stubSettled() storageMock.setSequence(session) - try engine.upgrade(topic: session.topic, permissions: Session.Permissions.stub()) + try engine.upgrade(topic: session.topic, permissions: permissions) XCTAssertTrue(relayMock.didCallRequest) + // TODO: Check permissions on stored session } func testUpgradeErrorSessionNotFound() { From 668746ab91bdbb514643fd73c52528bba0ee7766 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Vants?= Date: Fri, 28 Jan 2022 20:55:50 -0300 Subject: [PATCH 4/6] Upgrade handling on receiver side --- .../WalletConnect/Engine/SessionEngine.swift | 79 +++++++++---------- Sources/WalletConnect/Types/ReasonCode.swift | 8 ++ .../Types/Session/SessionPermissions.swift | 11 ++- .../Types/Session/SessionSequence.swift | 6 +- .../SessionEngineTests.swift | 62 +++++++++++++++ Tests/WalletConnectTests/Stub/Stubs.swift | 5 ++ 6 files changed, 126 insertions(+), 45 deletions(-) diff --git a/Sources/WalletConnect/Engine/SessionEngine.swift b/Sources/WalletConnect/Engine/SessionEngine.swift index 3fd54690f..d0da92398 100644 --- a/Sources/WalletConnect/Engine/SessionEngine.swift +++ b/Sources/WalletConnect/Engine/SessionEngine.swift @@ -216,6 +216,7 @@ final class SessionEngine { } func upgrade(topic: String, permissions: Session.Permissions) throws { + let permissions = SessionPermissions(permissions: permissions) guard var session = sequencesStore.getSequence(forTopic: topic) else { throw WalletConnectError.noSessionMatchingTopic(topic) } @@ -234,25 +235,6 @@ final class SessionEngine { relayer.request(.wcSessionUpgrade(SessionType.UpgradeParams(permissions: newPermissions)), onTopic: topic) } - private func validatePermissions(_ permissions: Session.Permissions) -> Bool { - for chainId in permissions.blockchains { - if !String.conformsToCAIP2(chainId) { - return false - } - } - for method in permissions.methods { - if method.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { - return false - } - } - for notification in permissions.notifications { - if notification.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { - return false - } - } - return true - } - func notify(topic: String, params: Session.Notification, completion: ((Error?)->())?) { guard let session = sequencesStore.getSequence(forTopic: topic), session.isSettled else { logger.debug("Could not find session for topic \(topic)") @@ -289,7 +271,7 @@ final class SessionEngine { case .sessionUpdate(let updateParams): handleSessionUpdate(payload: subscriptionPayload, updateParams: updateParams) case .sessionUpgrade(let upgradeParams): - handleSessionUpgrade(topic: topic, upgradeParams: upgradeParams, requestId: requestId) + handleSessionUpgrade(payload: subscriptionPayload, upgradeParams: upgradeParams) case .sessionDelete(let deleteParams): handleSessionDelete(deleteParams, topic: topic) case .sessionPayload(let sessionPayloadParams): @@ -357,40 +339,34 @@ final class SessionEngine { relayer.respondError(for: payload, reason: .unauthorizedMatchingController(isController: isController)) return } - session.settled?.state = updateParams.state sequencesStore.setSequence(session) relayer.respondSuccess(for: payload) onSessionUpdate?(topic, updateParams.state.accounts) } - private func handleSessionUpgrade(topic: String, upgradeParams: SessionType.UpgradeParams, requestId: Int64) { - guard var session = sequencesStore.getSequence(forTopic: topic) else { - logger.debug("Could not find session for topic \(topic)") + private func handleSessionUpgrade(payload: WCRequestSubscriptionPayload, upgradeParams: SessionType.UpgradeParams) { + guard validatePermissions(upgradeParams.permissions) else { + relayer.respondError(for: payload, reason: .invalidUpgradeRequest(context: .session)) return } - guard session.peerIsController else { - let error = WalletConnectError.unauthrorized(.unauthorizedUpgradeRequest) - logger.error(error) - respond(error: error, requestId: requestId, topic: topic) + guard var session = sequencesStore.getSequence(forTopic: payload.topic) else { + relayer.respondError(for: payload, reason: .noContextWithTopic(context: .session, topic: payload.topic)) return } - let permissions = Session.Permissions( - blockchains: upgradeParams.permissions.blockchain.chains, - methods: upgradeParams.permissions.jsonrpc.methods) - session.upgrade(permissions) - guard let newPermissions = session.settled?.permissions else { + guard session.peerIsController else { + relayer.respondError(for: payload, reason: .unauthorizedUpgradeRequest(context: .session)) return } - let response = JSONRPCResponse(id: requestId, result: AnyCodable(true)) - relayer.respond(topic: topic, response: JsonRpcResponseTypes.response(response)) { [unowned self] error in - if let error = error { - logger.error(error) - } else { - sequencesStore.setSequence(session) - onSessionUpgrade?(session.topic, newPermissions) - } + guard !isController else { + relayer.respondError(for: payload, reason: .unauthorizedMatchingController(isController: isController)) + return } + session.upgrade(upgradeParams.permissions) + sequencesStore.setSequence(session) + let newPermissions = session.settled!.permissions // We know session is settled + relayer.respondSuccess(for: payload) + onSessionUpgrade?(session.topic, newPermissions) } private func handleSessionPing(topic: String, requestId: Int64) { @@ -605,4 +581,25 @@ final class SessionEngine { logger.error("Peer failed to upgrade permissions.") } } + + private func validatePermissions(_ permissions: SessionPermissions) -> Bool { + for chainId in permissions.blockchain.chains { + if !String.conformsToCAIP2(chainId) { + return false + } + } + for method in permissions.jsonrpc.methods { + if method.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { + return false + } + } + if let notificationTypes = permissions.notifications?.types { + for notification in notificationTypes { + if notification.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { + return false + } + } + } + return true + } } diff --git a/Sources/WalletConnect/Types/ReasonCode.swift b/Sources/WalletConnect/Types/ReasonCode.swift index cfcbd03f3..187095d47 100644 --- a/Sources/WalletConnect/Types/ReasonCode.swift +++ b/Sources/WalletConnect/Types/ReasonCode.swift @@ -10,18 +10,22 @@ enum ReasonCode { // 1000 (Internal) case invalidUpdateRequest(context: Context) + case invalidUpgradeRequest(context: Context) case noContextWithTopic(context: Context, topic: String) // 3000 (Unauthorized) case unauthorizedUpdateRequest(context: Context) + case unauthorizedUpgradeRequest(context: Context) case unauthorizedMatchingController(isController: Bool) var code: Int { switch self { case .generic: return 0 case .invalidUpdateRequest: return 1003 + case .invalidUpgradeRequest: return 1004 case .noContextWithTopic: return 1301 case .unauthorizedUpdateRequest: return 3003 + case .unauthorizedUpgradeRequest: return 3004 case .unauthorizedMatchingController: return 3005 } } @@ -32,10 +36,14 @@ enum ReasonCode { return message case .invalidUpdateRequest(let context): return "Invalid \(context) update request" + case .invalidUpgradeRequest(let context): + return "Invalid \(context) upgrade request" case .noContextWithTopic(let context, let topic): return "No matching \(context) with topic: \(topic)" case .unauthorizedUpdateRequest(let context): return "Unauthorized \(context) update request" + case .unauthorizedUpgradeRequest(let context): + return "Unauthorized \(context) upgrade request" case .unauthorizedMatchingController(let isController): return "Unauthorized: peer is also \(isController ? "" : "non-")controller" } diff --git a/Sources/WalletConnect/Types/Session/SessionPermissions.swift b/Sources/WalletConnect/Types/Session/SessionPermissions.swift index dc65d9ef1..91489fcea 100644 --- a/Sources/WalletConnect/Types/Session/SessionPermissions.swift +++ b/Sources/WalletConnect/Types/Session/SessionPermissions.swift @@ -38,8 +38,13 @@ struct SessionPermissions: Codable, Equatable { self.controller = nil } - mutating func upgrade(with sessionPermissions: Session.Permissions) { - blockchain.chains.formUnion(sessionPermissions.blockchains) - jsonrpc.methods.formUnion(sessionPermissions.methods) +// mutating func upgrade(with sessionPermissions: Session.Permissions) { +// blockchain.chains.formUnion(sessionPermissions.blockchains) +// jsonrpc.methods.formUnion(sessionPermissions.methods) +// } + + mutating func upgrade(with permissions: SessionPermissions) { + blockchain.chains.formUnion(permissions.blockchain.chains) + jsonrpc.methods.formUnion(permissions.jsonrpc.methods) } } diff --git a/Sources/WalletConnect/Types/Session/SessionSequence.swift b/Sources/WalletConnect/Types/Session/SessionSequence.swift index 61d4048e3..3ee8c5922 100644 --- a/Sources/WalletConnect/Types/Session/SessionSequence.swift +++ b/Sources/WalletConnect/Types/Session/SessionSequence.swift @@ -74,7 +74,11 @@ struct SessionSequence: ExpirableSequence { return settled.permissions.jsonrpc.methods.contains(method) } - mutating func upgrade(_ permissions: Session.Permissions) { +// mutating func upgrade(_ permissions: Session.Permissions) { +// settled?.permissions.upgrade(with: permissions) +// } + + mutating func upgrade(_ permissions: SessionPermissions) { settled?.permissions.upgrade(with: permissions) } diff --git a/Tests/WalletConnectTests/SessionEngineTests.swift b/Tests/WalletConnectTests/SessionEngineTests.swift index c97b2ea0b..5dee3c56b 100644 --- a/Tests/WalletConnectTests/SessionEngineTests.swift +++ b/Tests/WalletConnectTests/SessionEngineTests.swift @@ -401,4 +401,66 @@ final class SessionEngineTests: XCTestCase { XCTAssertTrue(error.isUnauthorizedNonControllerCallError) } } + + // MARK: - Upgrade peer response tests + + func testUpgradePeerSuccess() { + setupEngine(isController: false) + var didCallbackUpgrade = false + let session = SessionSequence.stubSettled(isPeerController: true) + storageMock.setSequence(session) + engine.onSessionUpgrade = { topic, _ in + didCallbackUpgrade = true + XCTAssertEqual(topic, session.topic) + } + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic)) + XCTAssertTrue(didCallbackUpgrade) + XCTAssertTrue(relayMock.didRespondSuccess) + } + + func testUpgradePeerErrorInvalidPermissions() { + setupEngine(isController: false) + let invalidPermissions = SessionPermissions.stub(chains: [""]) + let session = SessionSequence.stubSettled(isPeerController: true) + storageMock.setSequence(session) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic, permissions: invalidPermissions)) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 1004) + } + + func testUpgradePeerErrorSessionNotFound() { + setupEngine(isController: false) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: "")) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 1301) + } + + func testUpgradePeerErrorSessionNotSettled() { + setupEngine(isController: false) + let session = SessionSequence.stubPreSettled(isPeerController: true) // Session is not fully settled + storageMock.setSequence(session) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic)) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 3004) + } + + func testUpgradePeerErrorUnauthorized() { + setupEngine(isController: false) + let session = SessionSequence.stubSettled() // Peer is not a controller + storageMock.setSequence(session) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic)) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 3004) + } + + func testUpgradePeerErrorMatchingController() { + setupEngine(isController: true) // Upgrade request received by a controller + let session = SessionSequence.stubSettled(isPeerController: true) + storageMock.setSequence(session) + subscriberMock.onReceivePayload?(WCRequestSubscriptionPayload.stubUpgrade(topic: session.topic)) + XCTAssertFalse(relayMock.didRespondSuccess) + XCTAssertEqual(relayMock.lastErrorCode, 3005) + } + + // TODO: Upgrade acknowledgement tests } diff --git a/Tests/WalletConnectTests/Stub/Stubs.swift b/Tests/WalletConnectTests/Stub/Stubs.swift index a3adf8321..11b405f85 100644 --- a/Tests/WalletConnectTests/Stub/Stubs.swift +++ b/Tests/WalletConnectTests/Stub/Stubs.swift @@ -64,4 +64,9 @@ extension WCRequestSubscriptionPayload { let updateMethod = WCMethod.wcSessionUpdate(SessionType.UpdateParams(accounts: accounts)).asRequest() return WCRequestSubscriptionPayload(topic: topic, wcRequest: updateMethod) } + + static func stubUpgrade(topic: String, permissions: SessionPermissions = SessionPermissions(permissions: Session.Permissions.stub())) -> WCRequestSubscriptionPayload { + let upgradeMethod = WCMethod.wcSessionUpgrade(SessionType.UpgradeParams(permissions: permissions)).asRequest() + return WCRequestSubscriptionPayload(topic: topic, wcRequest: upgradeMethod) + } } From fdcf02c276bb41362e624cbe567f18fe7eadc475 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Vants?= Date: Fri, 28 Jan 2022 20:57:34 -0300 Subject: [PATCH 5/6] Removed unnecessary integration test case --- Tests/IntegrationTests/ClientTest.swift | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/Tests/IntegrationTests/ClientTest.swift b/Tests/IntegrationTests/ClientTest.swift index e9a5e12f2..45f69ada5 100644 --- a/Tests/IntegrationTests/ClientTest.swift +++ b/Tests/IntegrationTests/ClientTest.swift @@ -266,31 +266,6 @@ final class ClientTests: XCTestCase { waitForExpectations(timeout: defaultTimeout, handler: nil) } - func testSessionUpgradeFailsOnNonControllerRequest() { - let proposerSessionUpgradeExpectation = expectation(description: "Proposer upgrades session") - proposerSessionUpgradeExpectation.isInverted = true - let responderSessionUpgradeExpectation = expectation(description: "Responder upgrades session") - responderSessionUpgradeExpectation.isInverted = true - let account = "0x022c0c42a80bd19EA4cF0F94c4F9F96645759716" - let permissions = Session.Permissions.stub() - let upgradePermissions = Session.Permissions(blockchains: ["eip155:42"], methods: ["eth_sendTransaction"]) - let uri = try! proposer.client.connect(sessionPermissions: permissions)! - try! responder.client.pair(uri: uri) - responder.onSessionProposal = { [unowned self] proposal in - self.responder.client.approve(proposal: proposal, accounts: [account]) - } - proposer.onSessionSettled = { [unowned self] sessionSettled in - try? proposer.client.upgrade(topic: sessionSettled.topic, permissions: upgradePermissions) - } - proposer.onSessionUpgrade = { topic, permissions in - proposerSessionUpgradeExpectation.fulfill() - } - responder.onSessionUpgrade = { topic, permissions in - responderSessionUpgradeExpectation.fulfill() - } - waitForExpectations(timeout: 3.0, handler: nil) - } - func testSuccessfulSessionUpdate() { let proposerSessionUpdateExpectation = expectation(description: "Proposer updates session on responder request") let responderSessionUpdateExpectation = expectation(description: "Responder updates session on proposer response") From d6e0f41613212195e27e4108de0528d9d9967f27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Vants?= Date: Tue, 1 Feb 2022 15:55:46 -0300 Subject: [PATCH 6/6] Add error descriptions --- Sources/WalletConnect/Engine/SessionEngine.swift | 2 +- .../Types/Session/SessionPermissions.swift | 5 ----- .../Types/Session/SessionSequence.swift | 4 ---- Sources/WalletConnect/WalletConnectError.swift | 12 +++++++++--- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/Sources/WalletConnect/Engine/SessionEngine.swift b/Sources/WalletConnect/Engine/SessionEngine.swift index ee90eeab5..529fc5810 100644 --- a/Sources/WalletConnect/Engine/SessionEngine.swift +++ b/Sources/WalletConnect/Engine/SessionEngine.swift @@ -219,7 +219,7 @@ final class SessionEngine { throw WalletConnectError.noSessionMatchingTopic(topic) } guard session.isSettled else { - throw WalletConnectError.sessionNotSettled + throw WalletConnectError.sessionNotSettled(topic) } guard isController else { throw WalletConnectError.unauthorizedNonControllerCall diff --git a/Sources/WalletConnect/Types/Session/SessionPermissions.swift b/Sources/WalletConnect/Types/Session/SessionPermissions.swift index 91489fcea..6f19280b1 100644 --- a/Sources/WalletConnect/Types/Session/SessionPermissions.swift +++ b/Sources/WalletConnect/Types/Session/SessionPermissions.swift @@ -38,11 +38,6 @@ struct SessionPermissions: Codable, Equatable { self.controller = nil } -// mutating func upgrade(with sessionPermissions: Session.Permissions) { -// blockchain.chains.formUnion(sessionPermissions.blockchains) -// jsonrpc.methods.formUnion(sessionPermissions.methods) -// } - mutating func upgrade(with permissions: SessionPermissions) { blockchain.chains.formUnion(permissions.blockchain.chains) jsonrpc.methods.formUnion(permissions.jsonrpc.methods) diff --git a/Sources/WalletConnect/Types/Session/SessionSequence.swift b/Sources/WalletConnect/Types/Session/SessionSequence.swift index 8dea2e67b..b54cf5158 100644 --- a/Sources/WalletConnect/Types/Session/SessionSequence.swift +++ b/Sources/WalletConnect/Types/Session/SessionSequence.swift @@ -73,10 +73,6 @@ struct SessionSequence: ExpirableSequence { return settled.permissions.jsonrpc.methods.contains(method) } -// mutating func upgrade(_ permissions: Session.Permissions) { -// settled?.permissions.upgrade(with: permissions) -// } - mutating func upgrade(_ permissions: SessionPermissions) { settled?.permissions.upgrade(with: permissions) } diff --git a/Sources/WalletConnect/WalletConnectError.swift b/Sources/WalletConnect/WalletConnectError.swift index 5d28c49dc..67ce2bd7a 100644 --- a/Sources/WalletConnect/WalletConnectError.swift +++ b/Sources/WalletConnect/WalletConnectError.swift @@ -6,7 +6,7 @@ import Foundation enum WalletConnectError: Error { case noSessionMatchingTopic(String) - case sessionNotSettled + case sessionNotSettled(String) case invalidPermissions case unauthorizedNonControllerCall @@ -56,12 +56,18 @@ extension WalletConnectError: CustomStringConvertible { var localizedDescription: String { switch self { + case .noSessionMatchingTopic(let topic): + return "No session found matching topic \(topic)." + case .sessionNotSettled(let topic): + return "Session is not settled on topic \(topic)." + case .invalidPermissions: + return "Permission set is invalid." + case .unauthorizedNonControllerCall: + return "Method must be called by a controller client." case .internal(let reason): return reason.description case .unauthrorized(let reason): return reason.description - default: - return "" } } }