-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Chat] sync chat sdk with kotlin implementation and add reject method #322
Conversation
llbartekll
commented
Jul 12, 2022
•
edited
Loading
edited
- sync chat sdk with kotlin implementation:
- rpc methods property names and structure
- timestamp format
- rpc methods serialisation
- add reject method
…V2 into chat-reject # Conflicts: # Example/IntegrationTests/Chat/ChatTests.swift # Sources/Chat/ProtocolServices/Invitee/InvitationHandlingService.swift # Sources/Chat/Types/InviteParams.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just few comments
|
||
class LeaveService { | ||
func leave(topic: String) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused?
@@ -52,7 +53,8 @@ class MessagingService { | |||
private func setUpRequestHandling() { | |||
networkingInteractor.requestPublisher.sink { [unowned self] subscriptionPayload in | |||
switch subscriptionPayload.request.params { | |||
case .message(let message): | |||
case .message(var message): | |||
message.topic = subscriptionPayload.topic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to do that? Is it possible to avoid struct mutation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wc_chatMessage
does not contain topic but when we query messages from storage it is convenient to distinguish them by topic.
maybe some storage improvement could allow to avoid this but instant sync with kotlin was a priority
will take this into consideration in next storage PR
|
||
try await networkingInteractor.respond(topic: responseTopic, response: response) | ||
|
||
invitePayloadStore.delete(forKey: inviteId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gj, need to do the same for approve
@@ -67,7 +69,7 @@ class InviteService { | |||
let inviteResponse = try jsonrpc.result.get(InviteResponse.self) | |||
logger.debug("Invite has been accepted") | |||
guard case .invite(let inviteParams) = response.requestParams else { return } | |||
Task { try await createThread(selfPubKeyHex: inviteParams.pubKey, peerPubKey: inviteResponse.pubKey, account: inviteParams.account, peerAccount: peerAccount)} | |||
Task { try await createThread(selfPubKeyHex: inviteParams.publicKey, peerPubKey: inviteResponse.publicKey, account: inviteParams.account, peerAccount: peerAccount)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use priority .background everywhere?
} else { | ||
throw Errors.decoding | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line