-
Notifications
You must be signed in to change notification settings - Fork 132
[PAY-3307] Upgrade chat blasts to chats in sdk #9438
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
Changes from all commits
da626ac
be559db
858d2d9
c5f5b09
1748b33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@audius/sdk': patch | ||
| --- | ||
|
|
||
| Upgrade chat blasts to real chats internally |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,13 @@ | ||
| package rpcz | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "net" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "comms.audius.co/discovery/misc" | ||
| "comms.audius.co/discovery/schema" | ||
| "github.com/gobwas/ws" | ||
| "github.com/gobwas/ws/wsutil" | ||
| "golang.org/x/exp/slog" | ||
|
|
@@ -96,3 +99,36 @@ func websocketPush(userId int32, payload []byte) { | |
| } | ||
|
|
||
| } | ||
|
|
||
| func websocketPushAll(rpcJson json.RawMessage, timestamp time.Time) { | ||
| mu.Lock() | ||
|
dharit-tan marked this conversation as resolved.
|
||
| defer mu.Unlock() | ||
|
|
||
| for _, s := range websockets { | ||
| encodedUserId, _ := misc.EncodeHashId(int(s.userId)) | ||
|
|
||
| data := struct { | ||
| RPC json.RawMessage `json:"rpc"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note this has the side effect of revealing that a blast is not a real DM for sleuthing eyes. I think this is fine given our strategy though. |
||
| Metadata schema.Metadata `json:"metadata"` | ||
| }{ | ||
| rpcJson, | ||
| schema.Metadata{Timestamp: timestamp.Format(time.RFC3339Nano), | ||
| // Note this is the userId of the user receiving the message | ||
| UserID: encodedUserId}, | ||
| } | ||
|
|
||
| payload, err := json.Marshal(data) | ||
| if err != nil { | ||
| logger.Warn("invalid websocket json " + err.Error()) | ||
| return | ||
| } | ||
|
|
||
| err = wsutil.WriteServerMessage(s.conn, ws.OpText, payload) | ||
| if err != nil { | ||
| logger.Info("websocket push failed: " + err.Error()) | ||
| removeWebsocket(s) | ||
| } else { | ||
| logger.Debug("websocket push all", "payload", string(payload)) | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import type { AuthService } from '../../services/Auth' | |
| import type { DiscoveryNodeSelectorService } from '../../services/DiscoveryNodeSelector/types' | ||
| import type { LoggerService } from '../../services/Logger' | ||
| import type { EventEmitterTarget } from '../../utils/EventEmitterTarget' | ||
| import { encodeHashId } from '../../utils/hashId' | ||
| import { parseParams } from '../../utils/parseParams' | ||
| import { | ||
| BaseAPI, | ||
|
|
@@ -64,10 +65,12 @@ import type { | |
| ChatMessage, | ||
| ChatWebsocketEventData, | ||
| RPCPayloadRequest, | ||
| ValidatedChatPermissions | ||
| ValidatedChatPermissions, | ||
| ChatBlast, | ||
| ChatCreateRPC | ||
| } from './serverTypes' | ||
|
|
||
| const GENERIC_MESSAGE_ERROR = 'Error: this message can not be displayed' | ||
| const GENERIC_MESSAGE_ERROR = 'Error: this message cannot be displayed' | ||
|
|
||
| export class ChatsApi | ||
| extends BaseAPI | ||
|
|
@@ -173,14 +176,18 @@ export class ChatsApi | |
| * @param params.limit the max number of chats to get | ||
| * @param params.before a timestamp cursor for pagination | ||
| * @param params.after a timestamp cursor for pagination | ||
| * @param params.currentUserId the user to act on behalf of | ||
| * @param params.userId the user to act on behalf of | ||
| * @returns the chat list response | ||
| */ | ||
| public async getAll(params?: ChatGetAllRequest) { | ||
| const { currentUserId, limit, before, after } = await parseParams( | ||
| const { userId, limit, before, after } = await parseParams( | ||
| 'getAll', | ||
| ChatGetAllRequestSchema | ||
| )(params) | ||
|
|
||
| // Get new blasts and upgrade them to chats | ||
| this.upgradeBlasts(userId) | ||
|
|
||
| const path = `/comms/chats` | ||
| const query: HTTPQuery = { | ||
| timestamp: new Date().getTime() | ||
|
|
@@ -194,8 +201,8 @@ export class ChatsApi | |
| if (after) { | ||
| query.after = after | ||
| } | ||
| if (currentUserId) { | ||
| query.current_user_id = currentUserId | ||
| if (userId) { | ||
| query.current_user_id = userId | ||
| } | ||
| const response = await this.signAndSendRequest({ | ||
| method: 'GET', | ||
|
|
@@ -282,6 +289,23 @@ export class ChatsApi | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Gets a list of chat blasts for which chats haven't been created yet | ||
| * @returns the blast messages list response | ||
| */ | ||
| public async getBlasts(): Promise<TypedCommsResponse<ChatBlast[]>> { | ||
|
dharit-tan marked this conversation as resolved.
|
||
| const query: HTTPQuery = { | ||
| timestamp: new Date().getTime() | ||
| } | ||
| const res = await this.signAndSendRequest({ | ||
| method: 'GET', | ||
| path: `/comms/blasts`, | ||
| headers: {}, | ||
| query | ||
| }) | ||
| return (await res.json()) as TypedCommsResponse<ChatBlast[]> | ||
| } | ||
|
|
||
| /** | ||
| * Gets the total unread message count for a user | ||
| * @param params.currentUserId the user to act on behalf of | ||
|
|
@@ -418,7 +442,7 @@ export class ChatsApi | |
| * @param params.currentUserId the user to act on behalf of | ||
| * @returns the rpc object | ||
| */ | ||
| public async create(params: ChatCreateRequest) { | ||
| public async create(params: ChatCreateRequest): Promise<ChatCreateRPC> { | ||
|
dharit-tan marked this conversation as resolved.
|
||
| const { currentUserId, userId, invitedUserIds } = await parseParams( | ||
| 'create', | ||
| ChatCreateRequestSchema | ||
|
|
@@ -428,14 +452,14 @@ export class ChatsApi | |
| const chatSecret = secp.utils.randomPrivateKey() | ||
| const invites = await this.createInvites(userId, invitedUserIds, chatSecret) | ||
|
|
||
| return await this.sendRpc({ | ||
| return (await this.sendRpc({ | ||
| current_user_id: currentUserId, | ||
| method: 'chat.create', | ||
| params: { | ||
| chat_id: chatId, | ||
| invites | ||
| } | ||
| }) | ||
| })) as ChatCreateRPC | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -767,6 +791,31 @@ export class ChatsApi | |
| return base64.decode(json.data) | ||
| } | ||
|
|
||
| private async upgradeBlasts(userId: string) { | ||
| const blasts = await this.getBlasts() | ||
| Promise.all( | ||
| blasts.data.map(async (blast) => { | ||
| const encodedSenderId = encodeHashId(blast.from_user_id) | ||
| if (encodedSenderId) { | ||
| await this.create({ | ||
| userId, | ||
| invitedUserIds: [encodedSenderId] | ||
| }) | ||
| this.eventEmitter.emit('message', { | ||
| chatId: blast.pending_chat_id, | ||
| message: { | ||
| message_id: blast.pending_chat_id + blast.chat_id, | ||
| message: blast.plaintext, | ||
| sender_user_id: encodedSenderId, | ||
| created_at: blast.created_at, | ||
| reactions: [] | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| private async getSignatureHeader(payload: string) { | ||
| const [allSignatureBytes, recoveryByte] = await this.auth.sign(payload) | ||
| const signatureBytes = new Uint8Array(65) | ||
|
|
@@ -853,6 +902,9 @@ export class ChatsApi | |
| created_at: data.metadata.timestamp | ||
| } | ||
| }) | ||
| } else if (data.rpc.method === 'chat.blast') { | ||
| const userId = data.metadata.userId | ||
| await this.upgradeBlasts(userId) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here @stereosteve
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait can't we use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh man... it was not passed in but that means the chats api is more stateful... thoughts? i could go either way.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohhh right right - forgot the websockets were associated via signature recovery. I think either way is fine! Though I wonder if we should remove
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah.... i kinda like the stateless idea. i'll give that a whirl and maybe remove listenUserId in a follow-up PR
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok so turns out my assumption about the user id attached to the metadata is wrong. so i'm going to merge what i have for now since it works and isn't n^2, and we can talk about refactoring later. a good solution maybe would be to add both senderUserId and receiverUserId to the metadata and then we can use whichever one on the client we need. |
||
| } | ||
| } | ||
| handleAsync() | ||
|
|
||
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.
This feels like it should be identical to the above except the
userIdslist comes from the current list of websockets not from the db? Why not use the same function in both places w/ a userIds param, or havewebsocketPushAllusewebsocketPush? There seems like there should be some reuse hereThere 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.
I guess
websocketPushhas the side effect of potentially sending the blast upgrade message multiple times within 10 seconds?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.
the thing i was trying to avoid was having this code reach into the websocket and grab the userId, felt like bad coupling. but i didn't think of
websocketPushAlljust callingwebsocketPush, that could work. we'd still have to form the metadata insidewebsocketPushAllbut that's probably ok.not sure what you're talking about with the 10 seconds thing?
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.
The code in
websocketPushcaches the recent messages for up to 10 seconds and resends them on each new message it looks likeThere 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.
ah i see it. i think that's still fine tho? that only happens if the websocket gets disconnected/reconnected right? and if multiple blast rpcs come in for the same blast message, the worst that'll happen is it'll trigger multiple calls to
getBlasts()but the data should all be fine since we upsert/don't insert dups etc.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.
hm one downside to
websocketPushAllcallingwebsocketPushis that we'll iterate through the websocket listn^2times... probably not the end of the world but not great.. dang should i favor perf or code re-use??