Deliver an invite #254

llbartekll · 2022-05-31T09:49:50Z

close #255

allows A to register at local registry
B resolves A by it's address
B sents an invite to A

sekimondre

I don't see any problems in merging if this is a proof-of-concept. It's good to keep all types internal until we need any of them to be public.

sekimondre · 2022-05-31T17:53:22Z

Sources/Chat/Types/ChatRequest.swift

+import Foundation
+import WalletConnectUtils
+
+struct ChatRequest: Codable {


There are too many RPC request duplicates already. I'll prioritize unifying the JSON-RPC types.

flypaper0 · 2022-06-01T09:07:50Z

Sources/Chat/Chat.swift

+                             logger: logger,
+                             topicToInvitationPubKeyStore: topicToInvitationPubKeyStore,
+                             inviteStore: inviteStore)
+        relayClient.socketConnectionStatusPublisher.sink { [unowned self] status in


Could we move subscriptions from init()? For example in subscriptions() method

flypaper0 · 2022-06-01T09:09:03Z

Sources/Chat/Chat.swift

+    let engine: Engine
+    let kms: KeyManagementService
+    var onInvite: ((Invite)->())?
+    var onNewThread: ((Thread)->())?


why you decide you use this style of callbacks? ("onInvite: ((Invite)->())?, onNewThread: ((Thread)->())?, onConnected: (()->())?")

As I know we take direction to async and publishers.
For example it could be subject whit returns:

enum Response { case invite(Invite) case newThread(Thread) }

or different publishers like we do in Sign

But I think one publisher which returns Enum is more convenient and easy to read

I think it will be good to have a singleton wrapper around this class so we can do client<->client tests
and that singleton will expose Combine publishers

I added many publisher to be consistent with Sign

but I am not sure if one publisher is better solution 🤔

flypaper0 · 2022-06-01T09:10:43Z

Sources/Chat/Chat.swift

+                onConnected?()
+            }
+        }.store(in: &publishers)
+        engine.onInvite = { [unowned self] invite in


We need to use single style of callbacks I think

for public interface definitely

I think that for 1to1 relations old school callbacks are good enough and for 1to_many relations it is best to use Combine as we do

so far engine's callbacks have always 1to1 relation with a client

flypaper0 · 2022-06-01T09:14:45Z

Sources/Chat/Chat.swift

+        self.registry = registry
+
+        self.kms = kms
+        let serialiser = Serializer(kms: kms)


Long inits. How about factories? And our init() will be like this:

init(registry: Registry, engine: Engine, kms: KeyManagementService) { self.registry = registry self.engine = engine self.kms = kms }

flypaper0 · 2022-06-01T09:17:52Z

Sources/Chat/Engine.swift

+    let registry: Registry
+    let logger: ConsoleLogging
+    let kms: KeyManagementService
+    let threadsStore = KeyValueStore<Thread>(defaults: RuntimeKeyValueStorage(), identifier: "threads")


default initializers is hard to test

flypaper0 · 2022-06-01T09:22:11Z

Sources/Chat/Engine.swift

+        self.logger = logger
+        self.inviteStore = inviteStore
+        self.topicToInvitationPubKeyStore = topicToInvitationPubKeyStore
+        networkingInteractor.responsePublisher.sink { [unowned self] response in


need to separate initializations and subscriptions

For example

init() { self.foo = foo self.bar = bar setupSubscriptions() }

yeah will do it before merge

flypaper0 · 2022-06-01T09:24:16Z

Sources/Chat/Engine.swift

+    func invite(account: Account) {
+        let peerPubKeyHex = registry.resolve(account: account)!
+        print("resolved pub key: \(peerPubKeyHex)")
+        let pubKey = try! kms.createX25519KeyPair()


safe unwraps?

will make invite throwing

flypaper0 · 2022-06-01T09:24:47Z

Sources/Chat/Engine.swift

+
+    func accept(inviteId: String) throws {
+        guard let hexPubKey = try topicToInvitationPubKeyStore.get(key: "todo-topic") else {
+            throw ChatError.noPublicKeyForInviteId


what is todo-topic?

there is no storage to persist and derive this topic yet

so it just indicates to replace it

flypaper0 · 2022-06-01T09:26:20Z

Sources/Chat/Engine.swift

+        let agreementKeys = try! kms.performKeyAgreement(selfPublicKey: pubKey, peerPublicKey: invite.pubKey)
+        let topic = agreementKeys.derivedTopic()
+        networkingInteractor.subscribe(topic: topic)
+        //TODO


// TODO what? : ]

to complete complete function implementation as it is not publishing yet

I will open another issue for accept etc.

flypaper0 · 2022-06-01T09:30:21Z

Sources/Chat/NetworkingInteractor.swift

+import WalletConnectRelay
+import WalletConnectUtils
+
+class NetworkingInteractor {


Is it possible to reuse NetworkInteractor from sign?

that is very good question.

I am afraid that for now it is more difficult to make it generic. And I already recognises some differences

Sign also depends to Sign WCrequests now - and few more

That is definitely something to consider but I think it can be time consuming and not worth it now

* 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>

llbartekll self-assigned this May 31, 2022

llbartekll requested review from flypaper0 and sekimondre May 31, 2022 10:12

llbartekll added the chat label May 31, 2022

llbartekll changed the title ~~Chat~~ Deliver an invite May 31, 2022

sekimondre approved these changes May 31, 2022

View reviewed changes

flypaper0 reviewed Jun 1, 2022

View reviewed changes

llbartekll requested a review from flypaper0 June 2, 2022 10:50

flypaper0 approved these changes Jun 2, 2022

View reviewed changes

llbartekll force-pushed the chat branch from dea4dc4 to 7bd7e55 Compare June 2, 2022 12:41

llbartekll added 11 commits June 2, 2022 15:29

Add Chat target, split packages

f321e4a

savepoint

958bfe8

Update networking interactor to decode unencrypted messages

61f609d

pass on invite test

cd751b0

restructure chat's project folder

9357ee3

Add engine storages

5094be1

extract storage domain identifiers

e4fdfe3

update logging

d28df7f

fix schemas

b36901c

Update style

e0bed55

Add publishers to Chat

fa8b379

llbartekll force-pushed the chat branch from 7bd7e55 to fa8b379 Compare June 2, 2022 13:30

rename kv store to codable strore

9f11396

llbartekll force-pushed the chat branch from acca040 to 9f11396 Compare June 3, 2022 06:38

llbartekll merged commit 49e22f2 into develop Jun 3, 2022

llbartekll deleted the chat branch June 3, 2022 06:52

flypaper0 mentioned this pull request Jun 9, 2022

Release v0.7.0-beta.101 #265

Merged

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

Deliver an invite #254

Deliver an invite #254

llbartekll commented May 31, 2022 •

edited

Loading

sekimondre left a comment

sekimondre May 31, 2022

flypaper0 Jun 1, 2022

llbartekll Jun 1, 2022

flypaper0 Jun 1, 2022

flypaper0 Jun 1, 2022

llbartekll Jun 1, 2022

llbartekll Jun 2, 2022

flypaper0 Jun 1, 2022

llbartekll Jun 1, 2022

flypaper0 Jun 1, 2022

flypaper0 Jun 1, 2022

llbartekll Jun 1, 2022

flypaper0 Jun 1, 2022

flypaper0 Jun 1, 2022

llbartekll Jun 1, 2022

flypaper0 Jun 1, 2022

llbartekll Jun 1, 2022

flypaper0 Jun 1, 2022

llbartekll Jun 1, 2022

flypaper0 Jun 1, 2022

llbartekll Jun 1, 2022

llbartekll Jun 1, 2022

flypaper0 Jun 1, 2022

llbartekll Jun 1, 2022

Deliver an invite #254

Deliver an invite #254

Conversation

llbartekll commented May 31, 2022 • edited Loading

sekimondre 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llbartekll commented May 31, 2022 •

edited

Loading